diff --git a/crates/jp_cli/src/cmd.rs b/crates/jp_cli/src/cmd.rs index a48e0914..f7d44bb1 100644 --- a/crates/jp_cli/src/cmd.rs +++ b/crates/jp_cli/src/cmd.rs @@ -9,7 +9,7 @@ pub(crate) mod plugin; mod query; pub(crate) mod target; pub(crate) mod time; -pub(crate) mod turn_range; +pub(crate) mod turn_selection; use std::{fmt, num::NonZeroU8}; diff --git a/crates/jp_cli/src/cmd/conversation/archive.rs b/crates/jp_cli/src/cmd/conversation/archive.rs index 9c3c1405..3d76adc1 100644 --- a/crates/jp_cli/src/cmd/conversation/archive.rs +++ b/crates/jp_cli/src/cmd/conversation/archive.rs @@ -25,8 +25,9 @@ use crate::{ /// Pass `--confirm` to prompt for every conversation, or `--no-confirm` / /// `--yes` to skip all prompts. /// -/// Use `--from`/`--until` to archive a range of conversations by creation date, -/// or `--inactive-since` to archive everything unused since a given time. +/// Use `--created-since`/`--created-before` to archive a range of conversations +/// by creation date, or `--inactive-since` to archive everything unused since a +/// given time. /// The three filters AND together when combined. /// /// Archived conversations are hidden from listings and pickers. @@ -37,15 +38,17 @@ pub(crate) struct Archive { #[command(flatten)] target: PositionalIds, - /// Archive all conversations created in a `[--from, --until)` range. + /// Archive all conversations created in a `[--created-since, + /// --created-before)` range. #[command(flatten)] range: CreationRange, /// Archive all conversations inactive since a given time. /// - /// Accepts the same formats as `--from`. + /// Accepts the same formats as `--created-since`. /// Filters on `last_activated_at` (when the conversation was last used) - /// rather than its creation date, which makes this distinct from `--until`. + /// rather than its creation date, which makes this distinct from the + /// `--created-since`/`--created-before` range. #[arg(long, conflicts_with = "id")] inactive_since: Option, diff --git a/crates/jp_cli/src/cmd/conversation/archive_tests.rs b/crates/jp_cli/src/cmd/conversation/archive_tests.rs index aa4b8971..66e4fbb6 100644 --- a/crates/jp_cli/src/cmd/conversation/archive_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/archive_tests.rs @@ -117,8 +117,8 @@ fn inactive_since_returns_no_load_request() { fn from_returns_no_load_request() { let cmd = Archive { range: CreationRange { - from: Some("1d".parse().unwrap()), - until: None, + since: Some("1d".parse().unwrap()), + before: None, }, ..empty_archive() }; @@ -131,8 +131,8 @@ fn from_returns_no_load_request() { fn until_returns_no_load_request() { let cmd = Archive { range: CreationRange { - from: None, - until: Some("1d".parse().unwrap()), + since: None, + before: Some("1d".parse().unwrap()), }, ..empty_archive() }; @@ -168,8 +168,8 @@ fn matches_inactive_since_uses_last_activated_at() { fn matches_filters_and_compose() { let cmd = Archive { range: CreationRange { - from: Some(ts(1000).into()), - until: Some(ts(2000).into()), + since: Some(ts(1000).into()), + before: Some(ts(2000).into()), }, inactive_since: Some(ts(5000).into()), ..empty_archive() @@ -234,8 +234,8 @@ fn resolve_filtered_composes_range_and_inactive_since() { let cmd = Archive { range: CreationRange { - from: Some(ts(1000).into()), - until: Some(ts(2000).into()), + since: Some(ts(1000).into()), + before: Some(ts(2000).into()), }, inactive_since: Some(ts(5000).into()), ..empty_archive() diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index a6c43b5d..8aeccdda 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -1,17 +1,15 @@ use std::{env, fs, path::PathBuf}; -use chrono::Utc; use jp_config::{ PartialAppConfig, conversation::compaction::{ CompactionConfig, CompactionRuleConfig, PartialCompactionRuleConfig, PartialSummaryConfig, - ReasoningMode, RuleBound, ToolCallsMode, + ReasoningMode, ToolCallsMode, }, }; use jp_conversation::{ - Compaction, CompactionRange, ConversationStream, RangeBound, ReasoningPolicy, SummaryPolicy, - ToolCallPolicy, - compaction::{extend_summary_range, resolve_range}, + Compaction, CompactionRange, ConversationStream, ReasoningPolicy, SummaryPolicy, + ToolCallPolicy, compaction::extend_summary_range, }; use jp_workspace::{ConversationHandle, ConversationMut, Workspace}; use tracing::warn; @@ -22,7 +20,9 @@ use crate::{ conversation_id::PositionalIds, lock::{LockOutcome, LockRequest, acquire_lock}, query::apply_model, - turn_range::{Bound, TurnRange}, + turn_selection::{ + Bound, BoundWindow, TurnSelection, keep_first_bound, keep_last_bound, resolve_window, + }, }, ctx::{Ctx, IntoPartialAppConfig}, format::compaction_policy_label, @@ -33,32 +33,16 @@ pub(crate) struct Compact { #[command(flatten)] target: PositionalIds, - /// Preserve the first N turns (or turns within a duration). - /// - /// Accepts a turn count (e.g. `2`) or a duration (e.g. `5h`). - /// Composes with `--first M`: the pair compacts the first M turns minus the - /// preserved prefix, e.g. `--keep-first 1 --first 16` compacts turns 2 - /// through 16. - #[arg(long, conflicts_with_all = ["from", "last", "turn"])] - keep_first: Option, - - /// Preserve the last N turns (or turns within a duration). - /// - /// Accepts a turn count (e.g. `3`) or a duration (e.g. `2h`). - /// Composes with `--last M`: the pair compacts the last M turns minus the - /// preserved suffix, e.g. `--keep-last 2 --last 16` compacts the 14 turns - /// before the final 2. - #[arg(long, conflicts_with_all = ["to", "first", "turn"])] - keep_last: Option, - /// Which turns to compact. /// - /// `--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 + /// `--from`/`--to` bound the compacted range; `--first N`/`--last N` + /// compact the first or last N turns (both together compact each window and + /// skip the middle); `--turn N` compacts a single turn, or `--turn A..B` an /// inclusive range (e.g. `1..5` is turns 1-5). + /// `--keep-first`/`--keep-last` protect turns at either end from whichever + /// range is selected. #[command(flatten)] - range: TurnRange, + range: TurnSelection, /// Strip reasoning (thinking) blocks from the compacted range. #[arg(short, long, conflicts_with = "compact")] @@ -133,34 +117,6 @@ pub(crate) struct Compact { } impl Compact { - /// Reject flag combinations clap cannot express. - /// - /// `--keep-first N --first M` means "compact the first M turns, minus the - /// first N" (and `--keep-last`/`--last` the mirror image at the end): with - /// N greater than M the preserved turns swallow the whole selection, so the - /// pair is rejected rather than silently selecting nothing. - /// Only count-based bounds are comparable up front; durations and the other - /// bound forms resolve against the stream and may legitimately come up - /// empty. - fn validate(&self) -> Result<(), String> { - if let (Some(RuleBound::Turns(keep)), Some(first)) = (&self.keep_first, self.range.first()) - && *keep > first - { - return Err(format!( - "--keep-first {keep} is greater than --first {first}: nothing would remain to \ - compact" - )); - } - if let (Some(RuleBound::Turns(keep)), Some(last)) = (&self.keep_last, self.range.last()) - && *keep > last - { - return Err(format!( - "--keep-last {keep} is greater than --last {last}: nothing would remain to compact" - )); - } - Ok(()) - } - /// Returns `true` if any dedicated policy flag is set. /// /// Policy flags (`--reasoning`/`--tools`/`--summarize`) build a single @@ -266,7 +222,8 @@ fn parse_tool_calls_mode(s: &str) -> Result { }) } -/// Resolve the turn range a single rule would compact. +/// Resolve the turn range a single rule would compact within one selected +/// window. /// /// `range_stream` is the baseline for resolving bounds, including /// `AfterLastCompaction` (`--from last-compaction`): it must be the stream as @@ -285,33 +242,29 @@ fn resolve_rule_range( range_stream: &ConversationStream, overlap_stream: &ConversationStream, rule: &CompactionRuleConfig, - from_override: Bound, - to_override: Bound, + window: &BoundWindow, + selection: &TurnSelection, ) -> Option { - // A CLI override (`--from`/`--to`/`--keep-first`/`--keep-last`) takes - // precedence; otherwise fall back to the rule's own bound. Either side - // resolving to `Empty` means nothing is compacted. - let from = match from_override { - Bound::Default => keep_first_to_bound(&rule.keep_first, range_stream), - other => other, + // The rule's own `keep_first`/`keep_last` fills a side the selection left + // open, unless the matching CLI keep flag is set: an explicit + // `--keep-first` replaces the configured bound rather than stacking on top + // of it. + let from = match (&window.from, selection.keep_first()) { + (Bound::Default, None) => keep_first_bound(&rule.keep_first, range_stream), + (side, _) => side.clone(), }; - let to = match to_override { - Bound::Default => keep_last_to_bound(&rule.keep_last, range_stream), - other => other, + let to = match (&window.to, selection.keep_last()) { + (Bound::Default, None) => keep_last_bound(&rule.keep_last, range_stream), + (side, _) => side.clone(), }; - let from = match from { - Bound::Empty => return None, - Bound::Default => None, - Bound::At(b) => Some(b), - }; - let to = match to { - Bound::Empty => return None, - Bound::Default => None, - Bound::At(b) => Some(b), + let resolved = resolve_window(&BoundWindow { from, to }, range_stream)?; + let trimmed = selection.trim(resolved, range_stream)?; + let range = CompactionRange { + from_turn: trimmed.from, + to_turn: trimmed.to, }; - let range = resolve_range(range_stream, from, to)?; Some(if rule.summary.is_some() { extend_summary_range(overlap_stream, range) } else { @@ -319,6 +272,49 @@ fn resolve_rule_range( }) } +/// Resolve every selected window into the ranges one rule will compact. +/// +/// A summary rule's range grows to subsume any summary it touches +/// ([`extend_summary_range`]), so two windows that started out disjoint can +/// extend onto each other — or onto the same pre-existing summary — and end +/// up overlapping or identical. +/// Coalescing here, before any text is generated, keeps one region from being +/// compacted twice and one summary from being generated only to be immediately +/// superseded by the next. +/// +/// Sorting by start turn first makes a single left-to-right sweep enough: +/// extending the open range as it goes absorbs transitive chains (A touches B, +/// B touches C) without a second pass. +fn plan_rule_ranges( + range_stream: &ConversationStream, + overlap_stream: &ConversationStream, + rule: &CompactionRuleConfig, + windows: &[BoundWindow], + selection: &TurnSelection, +) -> Vec { + let mut ranges: Vec = windows + .iter() + .filter_map(|window| { + resolve_rule_range(range_stream, overlap_stream, rule, window, selection) + }) + .collect(); + + ranges.sort_by_key(|r| (r.from_turn, r.to_turn)); + + let mut merged: Vec = Vec::with_capacity(ranges.len()); + for range in ranges { + match merged.last_mut() { + // Intersection only: two abutting ranges are distinct regions, and + // for a summary rule two adjacent summaries are legal. + Some(last) if range.from_turn <= last.to_turn => { + last.to_turn = last.to_turn.max(range.to_turn); + } + _ => merged.push(range), + } + } + merged +} + /// Generate the summary text (if any) and assemble a [`Compaction`] for an /// already-resolved range. /// @@ -358,14 +354,14 @@ async fn build_compaction_for_range( /// Build compaction events from the given resolved rules. /// -/// Each rule produces one `Compaction` event. -/// Runtime range overrides (`--from`/`--to`) apply to every rule. +/// Each rule produces one `Compaction` event per selected window, so a disjoint +/// selection (`--first N --last M`) compacts each window separately and leaves +/// the turns between them untouched. pub(crate) async fn build_compaction_events( events: &ConversationStream, cfg: &jp_config::AppConfig, rules: &[CompactionRuleConfig], - from_override: Bound, - to_override: Bound, + selection: &TurnSelection, printer: Option<&jp_printer::Printer>, ) -> crate::Result> { // Two distinct baselines: @@ -379,21 +375,15 @@ pub(crate) async fn build_compaction_events( // - `overlap` accumulates the compactions generated so far, so a later // summary rule's overlap extension sees earlier summaries in this same // invocation and can't be appended unextended. + let windows = selection.windows(events); let mut overlap = events.clone(); let mut compactions = Vec::new(); for rule in rules { - let Some(range) = resolve_rule_range( - events, - &overlap, - rule, - from_override.clone(), - to_override.clone(), - ) else { - continue; - }; - let compaction = build_compaction_for_range(events, cfg, rule, range, printer).await?; - overlap.add_compaction(compaction.clone()); - compactions.push(compaction); + for range in plan_rule_ranges(events, &overlap, rule, &windows, selection) { + let compaction = build_compaction_for_range(events, cfg, rule, range, printer).await?; + overlap.add_compaction(compaction.clone()); + compactions.push(compaction); + } } Ok(compactions) @@ -555,52 +545,6 @@ fn kept_line(verb: &str, from: usize, to: usize) -> String { } } -/// Convert a `keep_first` rule bound to a `from` [`Bound`]. -fn keep_first_to_bound(bound: &RuleBound, events: &ConversationStream) -> Bound { - match bound { - // "Keep first N" means compaction starts at turn 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 - // turn after `conversation_start + d`. A window covering the whole - // conversation preserves everything. - let Some(first) = events.iter().next() else { - return Bound::Empty; - }; - let Ok(d) = chrono::Duration::from_std(*d) else { - return Bound::Empty; - }; - match events.turn_at_time(first.event.timestamp + d) { - Some(turn) => Bound::At(RangeBound::Absolute(turn.index() + 1)), - None => Bound::At(RangeBound::Absolute(0)), - } - } - RuleBound::AfterLastCompaction => Bound::At(RangeBound::AfterLastCompaction), - } -} - -/// Convert a `keep_last` rule bound to a `to` [`Bound`]. -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)), - // `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. - match events.turn_at_time(Utc::now() - *d) { - Some(turn) => Bound::At(RangeBound::Absolute(turn.index())), - None => Bound::Empty, - } - } - RuleBound::AfterLastCompaction => Bound::Default, - } -} - /// Build a `Compaction` event from mechanical policies (no summary). fn build_mechanical_compaction( from_turn: usize, @@ -640,7 +584,18 @@ impl Compact { } pub(crate) async fn run(self, ctx: &mut Ctx, handles: Vec) -> Output { - self.validate()?; + self.range.validate()?; + + // Reject an out-of-range `--turn` against every target before compacting + // any of them, so a bad endpoint on the last conversation can't leave the + // earlier ones already compacted. + // `compact_one` re-checks against the locked snapshot, which is the + // authoritative count if a concurrent writer appends in between. + for handle in &handles { + self.range + .check_turn_range(ctx.workspace.events(handle)?.turn_count())?; + } + for handle in handles { self.compact_one(ctx, handle).await?; } @@ -681,18 +636,7 @@ 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()); - } + self.range.check_turn_range(events_snapshot.turn_count())?; // The effective rules combine the configured rules with any policy // flags / inline DSL (replace, or append under bare `--compact`). @@ -700,14 +644,8 @@ impl Compact { .effective_rules(&cfg) .map_err(|e| crate::error::Error::Compaction(e.to_string()))?; - // Range overrides (`--from`/`--to`/`--keep-first`/`--keep-last`) are - // resolved at runtime (they need the stream for duration and "last" - // resolution) and apply on top of every active rule. - let from_override = self.resolve_from(&events_snapshot); - let to_override = self.resolve_to(&events_snapshot); - if self.dry_run { - Self::preview_compaction(ctx, &events_snapshot, &rules, &from_override, &to_override); + Self::preview_compaction(ctx, &events_snapshot, &rules, &self.range); return Ok(()); } @@ -715,8 +653,7 @@ impl Compact { &events_snapshot, &cfg, &rules, - from_override, - to_override, + &self.range, Some(&ctx.printer), ) .await?; @@ -752,45 +689,40 @@ impl Compact { ctx: &Ctx, events_snapshot: &ConversationStream, rules: &[CompactionRuleConfig], - from_override: &Bound, - to_override: &Bound, + selection: &TurnSelection, ) { // Range resolution uses the original snapshot for every rule, while // `overlap` accumulates this run's summaries so later summary rules // preview the same (possibly extended) ranges as the real run. + let windows = selection.windows(events_snapshot); let mut overlap = events_snapshot.clone(); let mut new_segments = Vec::new(); for rule in rules { - let Some(range) = resolve_rule_range( - events_snapshot, - &overlap, - rule, - from_override.clone(), - to_override.clone(), - ) else { - continue; - }; - let label = if rule.summary.is_some() { - Some("summary".to_owned()) - } else { - compaction_policy_label(&build_mechanical_compaction( - range.from_turn, - range.to_turn, - rule, - )) - }; - new_segments.push(TimelineSegment { - from: range.from_turn, - to: range.to_turn, - label, - existing: false, - }); - if rule.summary.is_some() { - overlap.add_compaction( - Compaction::new(range.from_turn, range.to_turn).with_summary(SummaryPolicy { - summary: String::new(), - }), - ); + for range in plan_rule_ranges(events_snapshot, &overlap, rule, &windows, selection) { + let label = if rule.summary.is_some() { + Some("summary".to_owned()) + } else { + compaction_policy_label(&build_mechanical_compaction( + range.from_turn, + range.to_turn, + rule, + )) + }; + new_segments.push(TimelineSegment { + from: range.from_turn, + to: range.to_turn, + label, + existing: false, + }); + if rule.summary.is_some() { + overlap.add_compaction( + Compaction::new(range.from_turn, range.to_turn).with_summary( + SummaryPolicy { + summary: String::new(), + }, + ), + ); + } } } @@ -809,56 +741,6 @@ impl Compact { ctx.printer.println(line); } } - - /// 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. - /// `--first` is the exception: it composes with `--keep-first`, which then - /// supplies the start of the range while `--first` caps its end (via - /// [`resolve_to`]). - /// - /// [`resolve_to`]: Self::resolve_to - fn resolve_from(&self, events: &ConversationStream) -> Bound { - if let Some(bound) = &self.keep_first - && self.range.first().is_some() - { - return keep_first_to_bound(bound, events); - } - 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. - /// - /// 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. - /// `--last` is the exception: it composes with `--keep-last`, which then - /// supplies the end of the range while `--last` sets its start (via - /// [`resolve_from`]). - /// - /// [`resolve_from`]: Self::resolve_from - fn resolve_to(&self, events: &ConversationStream) -> Bound { - if let Some(bound) = &self.keep_last - && self.range.last().is_some() - { - return keep_last_to_bound(bound, events); - } - 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, - } - } } #[cfg(test)] diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index 0602f7ec..fa1733bd 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -10,15 +10,15 @@ use jp_config::{ model::{PartialModelConfig, id::PartialModelIdOrAliasConfig}, }; use jp_conversation::{ - Compaction, ConversationStream, RangeBound, ReasoningPolicy, ToolCallPolicy, + Compaction, ConversationStream, ReasoningPolicy, SummaryPolicy, ToolCallPolicy, event::{ToolCallRequest, ToolCallResponse}, }; use jp_printer::Printer; use serde_json::{Map, Value}; use super::{ - Bound, Compact, IntoPartialAppConfig as _, TimelineSegment, build_compaction_events, - existing_segments, segments_for_compactions, timeline_lines, + Compact, IntoPartialAppConfig as _, TimelineSegment, TurnSelection, build_compaction_events, + existing_segments, plan_rule_ranges, segments_for_compactions, timeline_lines, }; /// Parse a `Compact` from `jp conversation compact ` for flag tests. @@ -34,6 +34,19 @@ fn parse_compact(args: &[&str]) -> Compact { TestCli::try_parse_from(argv).unwrap().compact } +/// A rule that summarizes the whole selection, with both keep bounds open so +/// the rule's own bounds never narrow the window under test. +fn summary_rule() -> CompactionRuleConfig { + CompactionConfig::finalize_rules(vec![PartialCompactionRuleConfig { + keep_first: Some(RuleBound::Turns(0)), + keep_last: Some(RuleBound::Turns(0)), + summary: Some(PartialSummaryConfig::default()), + ..PartialCompactionRuleConfig::default() + }]) + .unwrap() + .remove(0) +} + #[test] fn bare_compact_flag_parses_without_a_value() { // Bare `--compact` (no value) means "apply config rules". @@ -279,8 +292,7 @@ fn tool_calls_mode_maps_to_policy() { &stream, &cfg, std::slice::from_ref(&rule), - Bound::Default, - Bound::Default, + &TurnSelection::default(), Some(&Printer::sink()), )) .unwrap(); @@ -311,8 +323,7 @@ fn keep_last_duration_covering_whole_conversation_compacts_nothing() { &stream, &cfg, std::slice::from_ref(&rule), - Bound::Default, - Bound::Default, + &TurnSelection::default(), Some(&Printer::sink()), )) .unwrap(); @@ -323,7 +334,7 @@ fn keep_last_duration_covering_whole_conversation_compacts_nothing() { } #[test] -fn from_last_resolves_against_original_stream_for_every_rule() { +fn from_last_compaction_resolves_against_original_stream_for_every_rule() { // `--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 @@ -350,13 +361,13 @@ fn from_last_resolves_against_original_stream_for_every_rule() { }, ]; + let compact = parse_compact(&["--from", "last-compaction"]); let compactions = runtime() .block_on(build_compaction_events( &stream, &cfg, &rules, - Bound::At(RangeBound::AfterLastCompaction), - Bound::Default, + &compact.range, Some(&Printer::sink()), )) .unwrap(); @@ -414,8 +425,7 @@ fn config_rule_strip_requests_blanks_args_through_projection() { &stream, &cfg, &rules, - Bound::Default, - Bound::Default, + &TurnSelection::default(), Some(&Printer::sink()), )) .unwrap(); @@ -469,16 +479,13 @@ fn keep_first_composes_with_first() { let compact = parse_compact(&["--keep-first", "1", "--first", "16", "-r"]); let cfg = AppConfig::new_test(); let rules = compact.effective_rules(&cfg).unwrap(); - let from = compact.resolve_from(&stream); - let to = compact.resolve_to(&stream); let compactions = runtime() .block_on(build_compaction_events( &stream, &cfg, &rules, - from, - to, + &compact.range, Some(&Printer::sink()), )) .unwrap(); @@ -500,16 +507,13 @@ fn keep_last_composes_with_last() { let compact = parse_compact(&["--keep-last", "2", "--last", "16", "-r"]); let cfg = AppConfig::new_test(); let rules = compact.effective_rules(&cfg).unwrap(); - let from = compact.resolve_from(&stream); - let to = compact.resolve_to(&stream); let compactions = runtime() .block_on(build_compaction_events( &stream, &cfg, &rules, - from, - to, + &compact.range, Some(&Printer::sink()), )) .unwrap(); @@ -518,21 +522,237 @@ fn keep_last_composes_with_last() { assert_eq!((compactions[0].from_turn, compactions[0].to_turn), (4, 17)); } +#[test] +fn first_and_last_compact_two_windows_and_skip_the_middle() { + // `--first 2 --last 2` over 8 turns compacts turns 1-2 and 7-8, leaving the + // four turns between them raw. Each window becomes its own compaction event. + let mut stream = ConversationStream::new_test(); + for t in 0..8 { + stream.start_turn(format!("turn {t}")); + } + + let compact = parse_compact(&["--first", "2", "--last", "2", "-r"]); + let cfg = AppConfig::new_test(); + let rules = compact.effective_rules(&cfg).unwrap(); + + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &rules, + &compact.range, + Some(&Printer::sink()), + )) + .unwrap(); + + let ranges: Vec<_> = compactions + .iter() + .map(|c| (c.from_turn, c.to_turn)) + .collect(); + assert_eq!(ranges, vec![(0, 1), (6, 7)]); +} + +#[test] +fn an_existing_summary_spanning_both_windows_plans_one_range() { + // An existing summary over turns 1-8 sits under both `--first 2` and + // `--last 2`. `extend_summary_range` grows each window onto it, so both + // become 1-8: without coalescing that is two LLM calls and two identical + // compactions, the second immediately superseding the first. + // + // Asserted at the planning seam, which is where the duplicate would be + // introduced; going through `build_compaction_events` would need a live + // summarizer. + let mut stream = ConversationStream::new_test(); + for t in 0..8 { + stream.start_turn(format!("turn {t}")); + } + stream.add_compaction(Compaction::new(0, 7).with_summary(SummaryPolicy { + summary: "existing".to_owned(), + })); + + let rule = summary_rule(); + let compact = parse_compact(&["--first", "2", "--last", "2"]); + let windows = compact.range.windows(&stream); + assert_eq!(windows.len(), 2, "the two windows start out disjoint"); + + let ranges = plan_rule_ranges(&stream, &stream, &rule, &windows, &compact.range); + + assert_eq!( + ranges + .iter() + .map(|r| (r.from_turn, r.to_turn)) + .collect::>(), + vec![(0, 7)], + "both windows extend onto the same existing summary, so they are one range" + ); +} + +#[test] +fn disjoint_windows_without_an_overlapping_summary_stay_separate() { + // The coalescing must not collapse windows that genuinely name distinct + // regions: with no summary to extend onto, `--first 2 --last 2` over 8 turns + // plans two ranges. + let mut stream = ConversationStream::new_test(); + for t in 0..8 { + stream.start_turn(format!("turn {t}")); + } + + let rule = summary_rule(); + let compact = parse_compact(&["--first", "2", "--last", "2"]); + let windows = compact.range.windows(&stream); + + let ranges = plan_rule_ranges(&stream, &stream, &rule, &windows, &compact.range); + + assert_eq!( + ranges + .iter() + .map(|r| (r.from_turn, r.to_turn)) + .collect::>(), + vec![(0, 1), (6, 7)] + ); +} + +#[test] +fn overlapping_first_and_last_windows_compact_once() { + // On a 3-turn conversation `--first 2 --last 2` covers every turn twice. + // Compaction acts per window, so two windows here would compact turn 2 + // twice — and for a summary rule, generate two overlapping summaries. + let mut stream = ConversationStream::new_test(); + for t in 0..3 { + stream.start_turn(format!("turn {t}")); + } + + let compact = parse_compact(&["--first", "2", "--last", "2", "-r"]); + let cfg = AppConfig::new_test(); + let rules = compact.effective_rules(&cfg).unwrap(); + + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &rules, + &compact.range, + Some(&Printer::sink()), + )) + .unwrap(); + + let ranges: Vec<_> = compactions + .iter() + .map(|c| (c.from_turn, c.to_turn)) + .collect(); + assert_eq!(ranges, vec![(0, 2)]); +} + +#[test] +fn abutting_first_and_last_windows_compact_once() { + // `--first 2 --last 2` over exactly 4 turns leaves no gap between the two + // windows, so they are one region rather than two touching ones. + let mut stream = ConversationStream::new_test(); + for t in 0..4 { + stream.start_turn(format!("turn {t}")); + } + + let compact = parse_compact(&["--first", "2", "--last", "2", "-r"]); + let cfg = AppConfig::new_test(); + let rules = compact.effective_rules(&cfg).unwrap(); + + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &rules, + &compact.range, + Some(&Printer::sink()), + )) + .unwrap(); + + let ranges: Vec<_> = compactions + .iter() + .map(|c| (c.from_turn, c.to_turn)) + .collect(); + assert_eq!(ranges, vec![(0, 3)]); +} + +#[test] +fn cli_keep_first_replaces_the_configured_keep_first() { + // An explicit `--keep-first 1` overrides the rule's `keep_first = 4` rather + // than stacking with it: the compacted range starts at turn 2 (index 1). + let mut stream = ConversationStream::new_test(); + for t in 0..8 { + stream.start_turn(format!("turn {t}")); + } + + let cfg = AppConfig::new_test(); + let rules = vec![CompactionRuleConfig { + keep_first: RuleBound::Turns(4), + keep_last: RuleBound::Turns(0), + reasoning: Some(ReasoningMode::Strip), + tool_calls: None, + summary: None, + }]; + + let compact = parse_compact(&["--keep-first", "1"]); + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &rules, + &compact.range, + Some(&Printer::sink()), + )) + .unwrap(); + + assert_eq!(compactions.len(), 1); + assert_eq!((compactions[0].from_turn, compactions[0].to_turn), (1, 7)); +} + +#[test] +fn keep_first_clamps_rather_than_shifts_an_explicit_from() { + // Turn 1 is already outside `--from 3`, so `--keep-first 1` has nothing to + // protect and the range is unchanged. + // `--to -1` pins the end via the CLI; left open, the ad-hoc `-r` rule's + // default `keep_last = 1` would supply it and obscure the start behaviour + // under test. + let mut stream = ConversationStream::new_test(); + for t in 0..8 { + stream.start_turn(format!("turn {t}")); + } + + let compact = parse_compact(&["--from", "3", "--to", "-1", "--keep-first", "1", "-r"]); + let cfg = AppConfig::new_test(); + let rules = compact.effective_rules(&cfg).unwrap(); + + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &rules, + &compact.range, + Some(&Printer::sink()), + )) + .unwrap(); + + assert_eq!(compactions.len(), 1); + assert_eq!((compactions[0].from_turn, compactions[0].to_turn), (2, 7)); +} + #[test] fn keep_first_greater_than_first_is_rejected() { // A preserved prefix larger than the selection is nonsensical and must be // an error rather than a silent no-op. let err = parse_compact(&["--keep-first", "17", "--first", "16"]) + .range .validate() .unwrap_err(); assert_eq!( err, - "--keep-first 17 is greater than --first 16: nothing would remain to compact" + "--keep-first 17 is greater than --first 16: nothing would remain to select" ); // Equal values select nothing, which is empty but not nonsensical. assert!( parse_compact(&["--keep-first", "16", "--first", "16"]) + .range .validate() .is_ok() ); @@ -543,16 +763,18 @@ fn keep_last_greater_than_last_is_rejected() { // A preserved suffix larger than the selection is nonsensical and must be // an error rather than a silent no-op. let err = parse_compact(&["--keep-last", "17", "--last", "16"]) + .range .validate() .unwrap_err(); assert_eq!( err, - "--keep-last 17 is greater than --last 16: nothing would remain to compact" + "--keep-last 17 is greater than --last 16: nothing would remain to select" ); // Equal values select nothing, which is empty but not nonsensical. assert!( parse_compact(&["--keep-last", "16", "--last", "16"]) + .range .validate() .is_ok() ); @@ -576,18 +798,23 @@ fn turn_out_of_range_is_rejected() { // 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) + parse_compact(&["--turn", "100"]) + .range + .check_turn_range(5) + .unwrap_err(), + "turn 100 out of range (conversation has 5 turns)" ); - assert_eq!( + assert!( parse_compact(&["--turn", "..100"]) .range - .turn_out_of_range(5), - Some(100) + .check_turn_range(5) + .is_err() ); - assert_eq!( - parse_compact(&["--turn", "3"]).range.turn_out_of_range(5), - None + assert!( + parse_compact(&["--turn", "3"]) + .range + .check_turn_range(5) + .is_ok() ); } diff --git a/crates/jp_cli/src/cmd/conversation/fork.rs b/crates/jp_cli/src/cmd/conversation/fork.rs index e242009d..84e2e9d7 100644 --- a/crates/jp_cli/src/cmd/conversation/fork.rs +++ b/crates/jp_cli/src/cmd/conversation/fork.rs @@ -6,7 +6,10 @@ use jp_workspace::{ConversationHandle, ConversationLock}; use tracing::debug; use crate::{ - cmd::{ConversationLoadRequest, Output, conversation_id::PositionalIds, time::TimeThreshold}, + cmd::{ + ConversationLoadRequest, Output, conversation_id::PositionalIds, + turn_selection::TurnSelection, + }, ctx::Ctx, }; @@ -18,37 +21,16 @@ pub(crate) struct Fork { #[arg(short, long, default_value = "false")] activate: bool, - /// Ignore all conversation events before the specified timestamp. + /// Which turns the fork inherits. /// - /// Inclusive: an event at exactly this timestamp is kept. - /// Timestamp can be relative (5days, 2mins, etc) or absolute. - /// Composes with `--until` to form a half-open `[from, until)` range. - #[arg(long)] - from: Option, - - /// Ignore all conversation events at or after the specified timestamp. - /// - /// Exclusive: an event at exactly this timestamp is dropped. - /// Timestamp can be relative (5days, 2mins, etc) or absolute. - /// Composes with `--from` to form a half-open `[from, until)` range. - #[arg(long)] - until: Option, - - /// Fork the first N turns of the conversation. - /// Defaults to 1. - /// - /// Can be combined with `--last` to keep both the leading and trailing - /// windows while dropping the turns in between. - #[arg(long, short = 'f')] - first: Option>, - - /// Fork the last N turns of the conversation. - /// Defaults to 1. - /// - /// Can be combined with `--first` to keep both the leading and trailing - /// windows while dropping the turns in between. - #[arg(long, short = 'l')] - last: Option>, + /// Without any selector, the fork inherits every turn. + /// `--from`/`--to` bound the inherited range; `--first N`/`--last N` + /// inherit the first or last N turns (both together keep each window and + /// drop the turns in between); `--turn N` inherits a single turn, or + /// `--turn A..B` an inclusive range. + /// `--keep-first`/`--keep-last` drop turns at either end of the selection. + #[command(flatten)] + range: TurnSelection, /// Fork without inheriting any turns. /// @@ -60,7 +42,9 @@ pub(crate) struct Fork { #[arg( short = 'N', long, - conflicts_with_all = ["from", "until", "first", "last", "compact"] + conflicts_with_all = [ + "from", "to", "turn", "first", "last", "keep_first", "keep_last", "compact", + ] )] no_turns: bool, @@ -79,6 +63,17 @@ impl Fork { } pub(crate) async fn run(self, ctx: &mut Ctx, handles: &[ConversationHandle]) -> Output { + self.range.validate()?; + + // Reject an out-of-range `--turn` against every source before forking any + // of them: `fork` accepts multiple sources, and checking inside the loop + // would leave the earlier forks created when a later source turns out to + // be too short. + for source in handles { + self.range + .check_turn_range(ctx.workspace.events(source)?.turn_count())?; + } + for source in handles { // `--no-turns` folds the source's effective config (base + every // delta) into a fresh base config; resolving it here lets the @@ -103,25 +98,17 @@ impl Fork { .with_created_at(events.created_at); return; } - // `retain` invalidates compaction overlays from the earliest - // removed turn onward (overlays confined to the untouched prefix - // survive), so a time filter that strips whole turns *or* events - // inside a surviving turn can't leave a stale overlay pointing at - // — or summarizing — content no longer in the fork. The - // `--first`/`--last` helpers below inherit the same guarantee. - events.retain(|event| { - self.from.is_none_or(|t| event.timestamp >= *t) - && self.until.is_none_or(|t| event.timestamp < *t) - }); - - let first = self.first.map(|v| v.unwrap_or(1)); - let last = self.last.map(|v| v.unwrap_or(1)); - match (first, last) { - (None, None) => {} - (Some(f), None) => events.retain_first_turns(f), - (None, Some(l)) => events.retain_last_turns(l), - (Some(f), Some(l)) => events.retain_first_and_last_turns(f, l), + if !self.range.is_set() { + return; } + + // `retain_turns` invalidates compaction overlays from the + // earliest removed turn onward (overlays confined to the + // untouched prefix survive), so a selection that drops turns + // can't leave a stale overlay pointing at — or summarizing — + // content no longer in the fork. + let selected = self.range.resolve(events); + events.retain_turns(|index| selected.contains(index)); })?; if self.compact.should_compact() { @@ -131,12 +118,13 @@ impl Fork { .compact .effective_rules(&cfg.conversation.compaction.rules) .map_err(|e| crate::error::Error::Compaction(e.to_string()))?; + // The fork's turn selection has already been applied to the + // stream, so compaction covers the whole fork. let compactions = super::compact::build_compaction_events( &events_snapshot, &cfg, &rules, - crate::cmd::turn_range::Bound::Default, - crate::cmd::turn_range::Bound::Default, + &TurnSelection::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/fork_tests.rs b/crates/jp_cli/src/cmd/conversation/fork_tests.rs index db943db1..b77e3aec 100644 --- a/crates/jp_cli/src/cmd/conversation/fork_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/fork_tests.rs @@ -22,6 +22,55 @@ use crate::{ cmd::{compact_flag::CompactFlag, conversation_id::PositionalIds}, }; +/// Parse a [`TurnSelection`] from the flags a user would pass to `jp c fork`. +/// +/// Going through clap keeps these cases pinned to the real flag surface rather +/// than to the struct's private fields. +fn selection(args: &[&str]) -> TurnSelection { + #[derive(clap::Parser)] + struct TestCli { + #[command(flatten)] + selection: TurnSelection, + } + + let mut argv = vec!["fork"]; + argv.extend_from_slice(args); + clap::Parser::try_parse_from(argv) + .map(|cli: TestCli| cli.selection) + .unwrap() +} + +/// A stream of `count` turns, each holding one request, two minutes apart +/// starting at 2020-01-01 00:00:00 UTC. +/// +/// Turn N starts at `00:(2N):00` and its request lands ten seconds later, so a +/// bound at an odd minute falls strictly inside a turn. +fn turns(count: u32) -> Vec { + (0..count) + .flat_map(|turn| { + [ + ConversationEvent::new( + TurnStart, + Utc.with_ymd_and_hms(2020, 1, 1, 0, turn * 2, 0).unwrap(), + ), + ConversationEvent::new( + ChatRequest::from(format!("Q{}", turn + 1)), + Utc.with_ymd_and_hms(2020, 1, 1, 0, turn * 2, 10).unwrap(), + ), + ] + }) + .collect() +} + +/// The request contents of a conversation, in stream order. +fn requests(stream: &ConversationStream) -> Vec { + stream + .iter() + .filter_map(|e| e.event.as_chat_request()) + .map(|r| r.content.clone()) + .collect() +} + #[test] #[expect(clippy::too_many_lines)] fn test_conversation_fork() { @@ -36,10 +85,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, - first: None, + range: TurnSelection::default(), title: None, compact: CompactFlag::default(), no_turns: false, @@ -81,10 +127,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, - first: None, + range: TurnSelection::default(), title: None, compact: CompactFlag::default(), no_turns: false, @@ -136,10 +179,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, - first: None, + range: TurnSelection::default(), title: None, compact: CompactFlag::default(), no_turns: true, @@ -194,10 +234,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: true, - from: None, - until: None, - last: None, - first: None, + range: TurnSelection::default(), title: None, compact: CompactFlag::default(), no_turns: false, @@ -251,10 +288,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: Some(Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap().into()), - until: None, - last: None, - first: None, + range: selection(&["--from", "2020-01-01T00:01:00Z"]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -269,48 +303,24 @@ fn test_conversation_fork() { let h = ctx.workspace.acquire_conversation(&id).unwrap(); let lock = ctx.workspace.test_lock(h); - lock.as_mut().update_events(|e| { - e.extend(vec![ - ConversationEvent::new( - ChatRequest::from("foo"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("bar"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("baz"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 2, 0).unwrap(), - ), - ]); - }); + lock.as_mut().update_events(|e| e.extend(turns(3))); id }, assert: |convs, _| { assert_eq!(convs.len(), 2); - assert_eq!(convs[0].2.iter().count(), 3); - // +1 for injected TurnStart - assert_eq!(convs[1].2.iter().count(), 3); - assert_eq!( - convs[0].2.first().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap() - ); - assert_eq!( - convs[1].2.first().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap() - ); + assert_eq!(requests(&convs[0].2), vec!["Q1", "Q2", "Q3"]); + // 00:01:00 falls inside turn 1 (00:00:00-00:02:00), so the fork + // starts at turn 2 — the first turn to begin after the cutoff. + // The bound never splits turn 1. + assert_eq!(requests(&convs[1].2), vec!["Q2", "Q3"]); }, }), - ("with until", TestCase { + ("with to", TestCase { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: Some(Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap().into()), - last: None, - first: None, + range: selection(&["--to", "2020-01-01T00:03:00Z"]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -325,49 +335,23 @@ fn test_conversation_fork() { let h = ctx.workspace.acquire_conversation(&id).unwrap(); let lock = ctx.workspace.test_lock(h); - lock.as_mut().update_events(|e| { - e.extend(vec![ - ConversationEvent::new( - ChatRequest::from("foo"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("bar"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("baz"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 2, 0).unwrap(), - ), - ]); - }); + lock.as_mut().update_events(|e| e.extend(turns(3))); id }, assert: |convs, _| { assert_eq!(convs.len(), 2); - assert_eq!(convs[0].2.iter().count(), 3); - // --until is exclusive: 00:01:00 event is dropped, only 00:00:00 survives. - // +1 for injected TurnStart. - assert_eq!(convs[1].2.iter().count(), 2); - assert_eq!( - convs[0].2.last().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 2, 0).unwrap() - ); - assert_eq!( - convs[1].2.last().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap() - ); + assert_eq!(requests(&convs[0].2), vec!["Q1", "Q2", "Q3"]); + // 00:03:00 falls inside turn 2 (00:02:00-00:04:00). `--to` is + // inclusive of the turn it lands in, so turn 2 is kept whole. + assert_eq!(requests(&convs[1].2), vec!["Q1", "Q2"]); }, }), ("with last (default 1)", TestCase { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: Some(None), - first: None, + range: selection(&["--last"]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -444,10 +428,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: Some(Some(2)), - first: None, + range: selection(&["--last", "2"]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -523,10 +504,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: Some(Some(10)), - first: None, + range: selection(&["--last", "10"]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -570,10 +548,7 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: Some(Some(1)), - first: None, + range: selection(&["--last", "1"]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -614,12 +589,9 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, + range: selection(&["--first"]), compact: CompactFlag::default(), no_turns: false, - first: Some(None), title: None, }, setup: |ctx| { @@ -693,12 +665,9 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: Some(Some(1)), + range: selection(&["--first", "2", "--last", "1"]), compact: CompactFlag::default(), no_turns: false, - first: Some(Some(2)), title: None, }, setup: |ctx| { @@ -765,12 +734,9 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, + range: selection(&["--first", "10"]), compact: CompactFlag::default(), no_turns: false, - first: Some(Some(10)), title: None, }, setup: |ctx| { @@ -812,12 +778,9 @@ fn test_conversation_fork() { args: Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, + range: TurnSelection::default(), compact: CompactFlag::default(), no_turns: false, - first: None, title: Some("my custom title".to_owned()), }, setup: |ctx| { @@ -841,14 +804,16 @@ fn test_conversation_fork() { assert_eq!(convs[1].1.title.as_deref(), Some("my custom title")); }, }), - ("with from and until", TestCase { + ("with from and to", TestCase { args: Fork { target: PositionalIds::default(), activate: false, - from: Some(Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap().into()), - until: Some(Utc.with_ymd_and_hms(2020, 1, 1, 0, 2, 0).unwrap().into()), - last: None, - first: None, + range: selection(&[ + "--from", + "2020-01-01T00:01:00Z", + "--to", + "2020-01-01T00:03:00Z", + ]), title: None, compact: CompactFlag::default(), no_turns: false, @@ -863,47 +828,75 @@ fn test_conversation_fork() { let h = ctx.workspace.acquire_conversation(&id).unwrap(); let lock = ctx.workspace.test_lock(h); - lock.as_mut().update_events(|e| { - e.extend(vec![ - ConversationEvent::new( - ChatRequest::from("foo"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("bar"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("baz"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 2, 0).unwrap(), - ), - ConversationEvent::new( - ChatResponse::message("qux"), - Utc.with_ymd_and_hms(2020, 1, 1, 0, 3, 0).unwrap(), - ), - ]); - }); + lock.as_mut().update_events(|e| e.extend(turns(4))); id }, assert: |convs, _| { assert_eq!(convs.len(), 2); - assert_eq!(convs[0].2.iter().count(), 4); - // `[from, until)` is half-open: 00:01:00 is kept (from is inclusive), - // 00:02:00 is dropped (until is exclusive). +1 for injected TurnStart. - assert_eq!(convs[1].2.iter().count(), 2); - assert_eq!( - convs[0].2.first().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap() - ); - assert_eq!( - convs[1].2.last().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 1, 0).unwrap() + assert_eq!(requests(&convs[0].2), vec!["Q1", "Q2", "Q3", "Q4"]); + // Both cutoffs fall mid-turn: `--from` starts at turn 2 and + // `--to` ends at turn 2, leaving exactly that turn. + assert_eq!(requests(&convs[1].2), vec!["Q2"]); + }, + }), + ("with turn range", TestCase { + args: Fork { + target: PositionalIds::default(), + activate: false, + range: selection(&["--turn", "2..3"]), + title: None, + compact: CompactFlag::default(), + no_turns: false, + }, + setup: |ctx| { + let id = ConversationId::try_from(ctx.now()).unwrap(); + ctx.workspace.create_conversation_with_id( + id, + Conversation::default().with_last_activated_at(ctx.now()), + ctx.config(), ); - assert_eq!( - convs[0].2.last().unwrap().event.timestamp, - Utc.with_ymd_and_hms(2020, 1, 1, 0, 3, 0).unwrap() + + let h = ctx.workspace.acquire_conversation(&id).unwrap(); + let lock = ctx.workspace.test_lock(h); + lock.as_mut().update_events(|e| e.extend(turns(5))); + id + }, + + assert: |convs, _| { + assert_eq!(convs.len(), 2); + // `--turn A..B` is inclusive on both ends. + assert_eq!(requests(&convs[1].2), vec!["Q2", "Q3"]); + }, + }), + ("with keep flags", TestCase { + args: Fork { + target: PositionalIds::default(), + activate: false, + range: selection(&["--keep-first", "1", "--keep-last", "1"]), + title: None, + compact: CompactFlag::default(), + no_turns: false, + }, + setup: |ctx| { + let id = ConversationId::try_from(ctx.now()).unwrap(); + ctx.workspace.create_conversation_with_id( + id, + Conversation::default().with_last_activated_at(ctx.now()), + ctx.config(), ); + + let h = ctx.workspace.acquire_conversation(&id).unwrap(); + let lock = ctx.workspace.test_lock(h); + lock.as_mut().update_events(|e| e.extend(turns(5))); + id + }, + + assert: |convs, _| { + assert_eq!(convs.len(), 2); + // The keep flags protect the first and last turns from the + // selection, so the fork inherits only the middle three. + assert_eq!(requests(&convs[1].2), vec!["Q2", "Q3", "Q4"]); }, }), ]; @@ -1055,10 +1048,7 @@ fn fork_targets_correct_source() { let fork = Fork { target: PositionalIds::default(), activate: false, - from: None, - until: None, - last: None, - first: None, + range: TurnSelection::default(), compact: CompactFlag::default(), no_turns: false, title: Some("forked-from-b".to_owned()), @@ -1107,6 +1097,86 @@ fn fork_targets_correct_source() { assert_eq!(a_requests, vec!["alpha question"]); } +/// An out-of-range `--turn` on *any* source must be rejected before *any* fork +/// is created. +/// +/// Validating inside the mutation loop would fork the first source, then error +/// on the second — leaving a conversation behind that the failed command +/// appears not to have created. +#[test] +fn turn_out_of_range_on_a_later_source_forks_nothing() { + let tmp = tempdir().unwrap(); + let (printer, _, _) = Printer::memory(OutputFormat::TextPretty); + let config = AppConfig::new_test(); + let storage = tmp.path().join(".jp"); + let user = tmp.path().join("user"); + let fs = Arc::new( + FsStorageBackend::new(&storage) + .unwrap() + .with_user_storage(&user, None, "abc") + .unwrap(), + ); + let workspace = Workspace::new(tmp.path()).with_backend(fs); + let mut ctx = Ctx::new( + workspace, + None, + Runtime::new().unwrap(), + Globals::default(), + config, + None, + printer, + ); + + // Source A has three turns, source B only one. + let id_a = ConversationId::try_from(ctx.now()).unwrap(); + ctx.workspace.create_conversation_with_id( + id_a, + Conversation::default().with_last_activated_at(ctx.now()), + ctx.config(), + ); + let h_a = ctx.workspace.acquire_conversation(&id_a).unwrap(); + let lock_a = ctx.workspace.test_lock(h_a); + lock_a.as_mut().update_events(|e| e.extend(turns(3))); + drop(lock_a); + + ctx.set_now(ctx.now() + Duration::from_secs(1)); + + let id_b = ConversationId::try_from(ctx.now()).unwrap(); + ctx.workspace.create_conversation_with_id( + id_b, + Conversation::default().with_last_activated_at(ctx.now()), + ctx.config(), + ); + let h_b = ctx.workspace.acquire_conversation(&id_b).unwrap(); + let lock_b = ctx.workspace.test_lock(h_b); + lock_b.as_mut().update_events(|e| e.extend(turns(1))); + drop(lock_b); + + ctx.set_now(ctx.now() + Duration::from_secs(1)); + + // Turn 3 exists in A but not in B. + let fork = Fork { + target: PositionalIds::default(), + activate: false, + range: selection(&["--turn", "3"]), + compact: CompactFlag::default(), + no_turns: false, + title: None, + }; + let handle_a = ctx.workspace.acquire_conversation(&id_a).unwrap(); + let handle_b = ctx.workspace.acquire_conversation(&id_b).unwrap(); + let result = tokio::runtime::Runtime::new() + .unwrap() + .block_on(fork.run(&mut ctx, &[handle_a, handle_b])); + + assert!(result.is_err(), "turn 3 is out of range for source B"); + assert_eq!( + ctx.workspace.conversations().count(), + 2, + "a rejected multi-source fork must not leave a partial fork behind" + ); +} + /// Regression test: forking a `--local` conversation must keep the fork /// local-only instead of projecting it into the workspace. #[test] diff --git a/crates/jp_cli/src/cmd/conversation/print.rs b/crates/jp_cli/src/cmd/conversation/print.rs index 6f36677e..c63e48f6 100644 --- a/crates/jp_cli/src/cmd/conversation/print.rs +++ b/crates/jp_cli/src/cmd/conversation/print.rs @@ -4,15 +4,14 @@ use jp_config::{ }, style::{reasoning::ReasoningDisplayConfig, typewriter::DelayDuration}, }; -use jp_conversation::{compaction::resolve_range, stream::TurnOrigin}; +use jp_conversation::stream::TurnOrigin; use jp_llm::tool::InvocationContext; use jp_workspace::ConversationHandle; use crate::{ cmd::{ - ConversationLoadRequest, Output, - conversation_id::PositionalIds, - turn_range::{Bound, TurnRange}, + ConversationLoadRequest, Output, conversation_id::PositionalIds, + turn_selection::TurnSelection, }, ctx::Ctx, render::{ConfigSource, TurnRenderer}, @@ -63,7 +62,7 @@ pub(crate) struct Print { /// /// Without any selector, prints the whole conversation. #[command(flatten)] - range: TurnRange, + range: TurnSelection, /// Use the current workspace config instead of the per-turn config. /// @@ -133,7 +132,7 @@ impl Print { fn print_conversation( ctx: &mut Ctx, handle: &ConversationHandle, - range: &TurnRange, + range: &TurnSelection, current_config: bool, print_style: Option, compacted: bool, @@ -193,43 +192,16 @@ impl Print { ); renderer.set_user_only(user_only); - // `--last 0` / `--first 0` explicitly selects nothing. - if range.is_empty() { - renderer.flush(); - return Ok(()); - } - - // `--turn` names specific turns; an out-of-range endpoint is an error. - if let Some(n) = range.turn_out_of_range(raw_count) { - return Err( - format!("turn {n} out of range (conversation has {raw_count} turns)").into(), - ); - } - - let from = match range.resolve_from(&events) { - Bound::Empty => { - renderer.flush(); - return Ok(()); - } - Bound::Default => None, - Bound::At(b) => Some(b), - }; - let to = match range.resolve_to(&events) { - Bound::Empty => { - renderer.flush(); - return Ok(()); - } - Bound::Default => None, - Bound::At(b) => Some(b), - }; + range.validate()?; + range.check_turn_range(raw_count)?; - // The selected raw 0-based turn range. A `from > to` or otherwise empty - // range selects nothing. Resolved against the raw stream, before - // projection, so it lines up with the header numbers. - let Some(selected) = resolve_range(&events, from, to) else { + // Resolved against the raw stream, before projection, so the selection + // lines up with the header numbers. + let selected = range.resolve(&events); + if selected.is_empty() { renderer.flush(); return Ok(()); - }; + } // Project for rendering when compacted; `origins` maps each rendered // turn back to the raw turn number(s) it represents for the header. @@ -245,7 +217,7 @@ impl Print { ); for (turn, origin) in events.iter_turns().zip(&origins) { - if origin.overlaps(selected.from_turn, selected.to_turn) { + if selected.overlaps_origin(*origin) { renderer.render_turn(&turn, *origin); } } diff --git a/crates/jp_cli/src/cmd/conversation/print_tests.rs b/crates/jp_cli/src/cmd/conversation/print_tests.rs index 4f5eecdb..3ad75d8e 100644 --- a/crates/jp_cli/src/cmd/conversation/print_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/print_tests.rs @@ -85,7 +85,7 @@ fn prints_user_message() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -114,7 +114,7 @@ fn prints_consecutive_reasoning_events_as_separate_blocks() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -140,7 +140,7 @@ fn prints_assistant_message() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -169,7 +169,7 @@ fn prints_reasoning_full() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -199,7 +199,7 @@ fn hides_reasoning_when_hidden() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -231,7 +231,7 @@ fn truncates_reasoning() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -272,7 +272,7 @@ fn prints_tool_call_and_result() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -300,7 +300,7 @@ fn prints_structured_data() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -331,7 +331,7 @@ fn structured_fence_is_closed_at_end_of_replay() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -370,7 +370,7 @@ fn structured_response_followed_by_message_closes_fence_first() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -419,7 +419,7 @@ fn prints_consecutive_structured_events_as_separate_fences() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -459,7 +459,7 @@ fn structured_to_message_in_same_turn_closes_fence_first() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -497,7 +497,7 @@ fn turn_separators_between_turns() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -525,7 +525,7 @@ fn turn_header_shows_turn_number_and_relative_time() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -580,7 +580,7 @@ fn turn_header_detail_on_assistant_first_turn() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -610,7 +610,7 @@ fn prints_conversation_by_id() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -633,7 +633,7 @@ fn empty_conversation_produces_no_content() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -685,7 +685,7 @@ fn full_conversation_round_trip() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -725,7 +725,7 @@ fn last_prints_only_last_turn() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(Some(1), None), + range: TurnSelection::from_last_turn(Some(1), None), current_config: false, style: None, compacted: false, @@ -766,7 +766,7 @@ fn last_two_with_three_turns() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(Some(2), None), + range: TurnSelection::from_last_turn(Some(2), None), current_config: false, style: None, compacted: false, @@ -795,7 +795,7 @@ fn last_exceeding_turn_count_prints_all() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(Some(5), None), + range: TurnSelection::from_last_turn(Some(5), None), current_config: false, style: None, compacted: false, @@ -840,7 +840,7 @@ fn blank_line_between_tool_calls_and_message() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -893,7 +893,7 @@ fn blank_line_between_message_and_tool_calls() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -962,7 +962,7 @@ fn no_extra_blank_line_between_consecutive_tool_calls() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -999,7 +999,7 @@ fn last_zero_prints_nothing() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(Some(0), None), + range: TurnSelection::from_last_turn(Some(0), None), current_config: false, style: None, compacted: false, @@ -1034,7 +1034,7 @@ fn turn_prints_specific_turn() { // Print only turn 2. let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, Some(2)), + range: TurnSelection::from_last_turn(None, Some(2)), current_config: false, style: None, compacted: false, @@ -1072,7 +1072,7 @@ fn turn_out_of_range_errors() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, Some(5)), + range: TurnSelection::from_last_turn(None, Some(5)), current_config: false, style: None, compacted: false, @@ -1091,7 +1091,7 @@ fn turn_zero_errors() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, Some(0)), + range: TurnSelection::from_last_turn(None, Some(0)), current_config: false, style: None, compacted: false, @@ -1136,7 +1136,7 @@ fn style_brief_hides_reasoning_and_tool_details() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Brief), compacted: false, @@ -1213,7 +1213,7 @@ fn style_chat_hides_reasoning_and_tool_calls() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Chat), compacted: false, @@ -1292,7 +1292,7 @@ fn style_user_shows_only_user_messages() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::User), compacted: false, @@ -1337,7 +1337,7 @@ fn role_header_renders_user_label_from_author() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1362,7 +1362,7 @@ fn role_header_falls_back_to_user_label_without_author() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1387,7 +1387,7 @@ fn role_header_renders_assistant_label_with_model_suffix() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1413,7 +1413,7 @@ fn role_header_assistant_appears_once_per_turn() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1455,7 +1455,7 @@ fn role_header_assistant_emitted_before_first_tool_call() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1486,7 +1486,7 @@ fn role_header_does_not_emit_plain_hr_separator() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1536,7 +1536,7 @@ fn style_chat_separates_messages_across_hidden_reasoning() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Chat), compacted: false, @@ -1587,7 +1587,7 @@ fn style_chat_separates_messages_across_hidden_tool_call() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Chat), compacted: false, @@ -1648,7 +1648,7 @@ fn style_full_shows_reasoning_and_untruncated_results() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Full), compacted: false, @@ -1718,7 +1718,7 @@ fn replay_shades_tool_chrome_after_reasoning() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1767,7 +1767,7 @@ fn replay_does_not_shade_tool_chrome_when_extension_disabled() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1817,7 +1817,7 @@ fn replay_suppresses_tool_chrome_when_show_disabled() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1886,7 +1886,7 @@ fn replay_keeps_the_gap_after_a_result_when_reasoning_renders_nothing() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(None, None), + range: TurnSelection::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1935,7 +1935,7 @@ fn replay_does_not_leak_reasoning_region_across_turns() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - range: TurnRange::from_last_turn(Some(2), None), + range: TurnSelection::from_last_turn(Some(2), None), current_config: true, style: None, compacted: false, diff --git a/crates/jp_cli/src/cmd/conversation/rm.rs b/crates/jp_cli/src/cmd/conversation/rm.rs index c64a59cd..6a0d7f71 100644 --- a/crates/jp_cli/src/cmd/conversation/rm.rs +++ b/crates/jp_cli/src/cmd/conversation/rm.rs @@ -23,7 +23,8 @@ pub(crate) struct Rm { #[command(flatten)] target: PositionalIds, - /// Remove all conversations created in a `[--from, --until)` range. + /// Remove all conversations created in a `[--created-since, + /// --created-before)` range. #[command(flatten)] range: CreationRange, @@ -76,9 +77,9 @@ impl Rm { /// /// Extracted from `run` so the filter step can be exercised in tests /// without driving the async confirmation/lock path. - /// This is the load-bearing line between `--from/--until` and actual - /// deletion; regressing it would silently turn a range delete into a full - /// wipe. + /// This is the dividing line between `--created-since`/`--created-before` + /// and actual deletion; regressing it would silently turn a range delete + /// into a full wipe. fn resolve_filtered( &self, workspace: &Workspace, diff --git a/crates/jp_cli/src/cmd/conversation/rm_tests.rs b/crates/jp_cli/src/cmd/conversation/rm_tests.rs index 2cf2917d..6addf87b 100644 --- a/crates/jp_cli/src/cmd/conversation/rm_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/rm_tests.rs @@ -38,8 +38,8 @@ fn workspace_with_conversations(ids: &[ConversationId]) -> Workspace { fn load_request_is_none_when_range_set() { let cmd = Rm { range: CreationRange { - from: Some(ts(1000).into()), - until: None, + since: Some(ts(1000).into()), + before: None, }, ..empty_rm() }; @@ -47,8 +47,8 @@ fn load_request_is_none_when_range_set() { let cmd = Rm { range: CreationRange { - from: None, - until: Some(ts(1000).into()), + since: None, + before: Some(ts(1000).into()), }, ..empty_rm() }; @@ -78,8 +78,8 @@ fn resolve_filtered_half_open_range_returns_only_in_range_ids() { let cmd = Rm { range: CreationRange { - from: Some(ts(1000).into()), - until: Some(ts(2000).into()), + since: Some(ts(1000).into()), + before: Some(ts(2000).into()), }, ..empty_rm() }; @@ -104,8 +104,8 @@ fn resolve_filtered_from_only_includes_from_inclusive() { let cmd = Rm { range: CreationRange { - from: Some(ts(1000).into()), - until: None, + since: Some(ts(1000).into()), + before: None, }, ..empty_rm() }; @@ -130,8 +130,8 @@ fn resolve_filtered_until_only_excludes_until_exclusive() { let cmd = Rm { range: CreationRange { - from: None, - until: Some(ts(2000).into()), + since: None, + before: Some(ts(2000).into()), }, ..empty_rm() }; @@ -153,8 +153,8 @@ fn resolve_filtered_empty_when_nothing_matches() { let cmd = Rm { range: CreationRange { - from: Some(ts(1000).into()), - until: Some(ts(2000).into()), + since: Some(ts(1000).into()), + before: Some(ts(2000).into()), }, ..empty_rm() }; diff --git a/crates/jp_cli/src/cmd/conversation/use_.rs b/crates/jp_cli/src/cmd/conversation/use_.rs index 07d37e59..8b22a2d6 100644 --- a/crates/jp_cli/src/cmd/conversation/use_.rs +++ b/crates/jp_cli/src/cmd/conversation/use_.rs @@ -18,9 +18,9 @@ use crate::{ /// /// Without flags, `jp c use [ID]` activates the given conversation (or opens a /// picker when no target is provided). -/// `--grep` and `--from`/`--until` restrict the picker's candidate set; when -/// the combined filter leaves a single conversation, it is activated directly -/// without prompting. +/// `--grep` and `--created-since`/`--created-before` restrict the picker's +/// candidate set; when the combined filter leaves a single conversation, it is +/// activated directly without prompting. #[derive(Debug, clap::Args)] pub(crate) struct Use { #[command(flatten)] @@ -33,7 +33,7 @@ pub(crate) struct Use { /// Case-insensitive unless the pattern contains an uppercase character /// (smart-case). /// Composable with target keywords (`?`, `?p`, `?s`, `?a`) and with - /// `--from` / `--until`. + /// `--created-since` / `--created-before`. #[arg(long)] grep: Option, @@ -51,7 +51,8 @@ impl Use { .any(ConversationTarget::is_archived) } - /// Whether any candidate-set filter (`--grep`, `--from`, `--until`) is set. + /// Whether any candidate-set filter (`--grep`, `--created-since`, + /// `--created-before`) is set. /// When true, `Use` resolves its handle internally instead of going through /// the standard pipeline. fn has_filter(&self) -> bool { diff --git a/crates/jp_cli/src/cmd/conversation/use_tests.rs b/crates/jp_cli/src/cmd/conversation/use_tests.rs index 76599510..acf858cf 100644 --- a/crates/jp_cli/src/cmd/conversation/use_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/use_tests.rs @@ -145,7 +145,7 @@ fn run_with_contention_skips_metadata_bump() { ); } -// --- Filter mode (`--grep`, `--from`, `--until`) ---------------------------- +// --- Filter mode (`--grep`, `--created-since`, `--created-before`) ---------- // // Filter mode resolves handles internally instead of going through the // standard pipeline. These tests exercise the N=1 short-circuit path that @@ -285,8 +285,8 @@ fn filter_range_narrows_then_grep_matches() { target: PositionalIds::from_targets(vec![]), grep: Some("shared-marker".into()), range: CreationRange { - from: Some(cutoff), - until: None, + since: Some(cutoff), + before: None, }, }; cmd.run(&mut ctx, vec![]).unwrap(); @@ -317,10 +317,10 @@ fn filter_mode_skips_standard_resolution() { target: PositionalIds::from_targets(vec![]), grep: None, range: CreationRange { - from: Some(TimeThreshold::from( + since: Some(TimeThreshold::from( Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap(), )), - until: None, + before: None, }, }; assert!(cmd.conversation_load_request().targets.is_none()); diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index 995d661f..1ff9f0a7 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -1049,8 +1049,9 @@ impl Query { &events, cfg, &rules, - crate::cmd::turn_range::Bound::Default, - crate::cmd::turn_range::Bound::Default, + // `--compact` on a query applies the configured rules to the whole + // conversation; there are no turn-selection flags to honour. + &crate::cmd::turn_selection::TurnSelection::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/time.rs b/crates/jp_cli/src/cmd/time.rs index 7232e064..40c2e269 100644 --- a/crates/jp_cli/src/cmd/time.rs +++ b/crates/jp_cli/src/cmd/time.rs @@ -48,7 +48,14 @@ impl FromStr for TimeThreshold { // Try as relative duration (e.g. "3w", "30d", "6h"). if let Ok(dur) = humantime::parse_duration(s) { - let cutoff = Utc::now() - dur; + // `humantime` accepts durations far outside chrono's representable + // range, and subtracting one panics rather than saturating, so an + // oversized value has to fail as a parse error instead of aborting + // the process. + let cutoff = chrono::Duration::from_std(dur) + .ok() + .and_then(|dur| Utc::now().checked_sub_signed(dur)) + .ok_or_else(|| format!("invalid time threshold '{s}': duration is too large"))?; return Ok(Self(cutoff)); } @@ -73,14 +80,18 @@ impl FromStr for TimeThreshold { } } -/// A half-open `[from, until)` filter on conversation creation date, shared +/// A half-open `[since, before)` filter on conversation creation date, shared /// between subcommands that want range-based selection (`jp c rm`, `jp c /// archive`, `jp c use`, …). /// -/// `--from` is inclusive, `--until` is exclusive. +/// `--created-since` is inclusive, `--created-before` is exclusive. /// Both accept the full [`TimeThreshold`] syntax (conversation ID, relative /// duration, or absolute date). /// +/// The flags name what they filter because `--from`/`--to` select *turns* +/// within a conversation elsewhere in the CLI (see `cmd::turn_selection`), and +/// the two accept near-identical values. +/// /// # Type parameters /// /// - `EXCLUSIVE`: whether the range is mutually exclusive with the positional @@ -95,15 +106,15 @@ impl FromStr for TimeThreshold { /// manually. #[derive(Debug)] pub(crate) struct CreationRange { - pub from: Option, - pub until: Option, + pub since: Option, + pub before: Option, } impl Default for CreationRange { fn default() -> Self { Self { - from: None, - until: None, + since: None, + before: None, } } } @@ -111,13 +122,13 @@ impl Default for CreationRange { impl CreationRange { /// Whether either bound is set. pub fn is_set(&self) -> bool { - self.from.is_some() || self.until.is_some() + self.since.is_some() || self.before.is_some() } /// Half-open range test on the conversation's creation timestamp. pub fn matches(&self, id: ConversationId) -> bool { - self.from.is_none_or(|t| id.timestamp() >= *t) - && self.until.is_none_or(|t| id.timestamp() < *t) + self.since.is_none_or(|t| id.timestamp() >= *t) + && self.before.is_none_or(|t| id.timestamp() < *t) } } @@ -145,36 +156,37 @@ impl TypedValueParser for TimeThresholdParser { impl clap::Args for CreationRange { fn augment_args(cmd: Command) -> Command { - let from_help = "Match conversations created at or after the specified time."; - let from_long_help = "Match conversations created at or after the specified \ - time.\n\nAccepts a conversation ID (uses its creation timestamp), a \ - relative duration (e.g. `3w`, `30d`, `6h`), or an absolute date \ - (e.g. `2026-01-01`). Composable with `--until`."; - let until_help = "Match conversations created before the specified time."; - let until_long_help = "Match conversations created before the specified time.\n\nAccepts \ - the same formats as `--from`. The range is half-open (`--until` is \ - exclusive), so `--from X --until Y` matches everything in `[X, Y)`."; + let since_help = "Match conversations created at or after the specified time."; + let since_long_help = "Match conversations created at or after the specified \ + time.\n\nAccepts a conversation ID (uses its creation timestamp), \ + a relative duration (e.g. `3w`, `30d`, `6h`), or an absolute date \ + (e.g. `2026-01-01`). Composable with `--created-before`."; + let before_help = "Match conversations created before the specified time."; + let before_long_help = "Match conversations created before the specified time.\n\nAccepts \ + the same formats as `--created-since`. The range is half-open \ + (`--created-before` is exclusive), so `--created-since X \ + --created-before Y` matches everything in `[X, Y)`."; let mut group = ArgGroup::new("creation_range") .multiple(true) - .args(["from", "until"]); + .args(["created_since", "created_before"]); if EXCLUSIVE { group = group.conflicts_with("id"); } cmd.arg( - Arg::new("from") - .long("from") + Arg::new("created_since") + .long("created-since") .value_parser(TimeThresholdParser) - .help(from_help) - .long_help(from_long_help), + .help(since_help) + .long_help(since_long_help), ) .arg( - Arg::new("until") - .long("until") + Arg::new("created_before") + .long("created-before") .value_parser(TimeThresholdParser) - .help(until_help) - .long_help(until_long_help), + .help(before_help) + .long_help(before_long_help), ) .group(group) } @@ -187,17 +199,17 @@ impl clap::Args for CreationRange { impl FromArgMatches for CreationRange { fn from_arg_matches(matches: &ArgMatches) -> Result { Ok(Self { - from: matches.get_one::("from").copied(), - until: matches.get_one::("until").copied(), + since: matches.get_one::("created_since").copied(), + before: matches.get_one::("created_before").copied(), }) } fn update_from_arg_matches(&mut self, matches: &ArgMatches) -> Result<(), clap::Error> { - if let Some(v) = matches.get_one::("from").copied() { - self.from = Some(v); + if let Some(v) = matches.get_one::("created_since").copied() { + self.since = Some(v); } - if let Some(v) = matches.get_one::("until").copied() { - self.until = Some(v); + if let Some(v) = matches.get_one::("created_before").copied() { + self.before = Some(v); } Ok(()) } diff --git a/crates/jp_cli/src/cmd/time_tests.rs b/crates/jp_cli/src/cmd/time_tests.rs index a560ab60..30c3c086 100644 --- a/crates/jp_cli/src/cmd/time_tests.rs +++ b/crates/jp_cli/src/cmd/time_tests.rs @@ -27,6 +27,18 @@ fn parse_relative_duration_days() { assert!((diff.num_days() - 30).abs() <= 1); } +#[test] +fn parse_rejects_a_duration_chrono_cannot_represent() { + // `humantime` parses durations far larger than chrono's date range, and + // subtracting one from `now` panics. `--created-since 10000000000000s` must + // return a parse error rather than abort the process. + let err = "10000000000000s".parse::().unwrap_err(); + assert!( + err.contains("too large"), + "expected a too-large error, got: {err}" + ); +} + #[test] fn parse_relative_duration_hours() { let t: TimeThreshold = "6h".parse().unwrap(); @@ -105,23 +117,23 @@ fn creation_range_is_set() { assert!(!empty.is_set()); let from_only: CreationRange = CreationRange { - from: Some(ts(1000).into()), - until: None, + since: Some(ts(1000).into()), + before: None, }; assert!(from_only.is_set()); let until_only: CreationRange = CreationRange { - from: None, - until: Some(ts(1000).into()), + since: None, + before: Some(ts(1000).into()), }; assert!(until_only.is_set()); } #[test] -fn creation_range_from_inclusive() { +fn creation_range_since_is_inclusive() { let range: CreationRange = CreationRange { - from: Some(ts(1000).into()), - until: None, + since: Some(ts(1000).into()), + before: None, }; // Strictly before: excluded. @@ -133,10 +145,10 @@ fn creation_range_from_inclusive() { } #[test] -fn creation_range_until_exclusive() { +fn creation_range_before_is_exclusive() { let range: CreationRange = CreationRange { - from: None, - until: Some(ts(2000).into()), + since: None, + before: Some(ts(2000).into()), }; assert!(range.matches(make_id(1999))); @@ -148,8 +160,8 @@ fn creation_range_until_exclusive() { #[test] fn creation_range_half_open() { let range: CreationRange = CreationRange { - from: Some(ts(1000).into()), - until: Some(ts(2000).into()), + since: Some(ts(1000).into()), + before: Some(ts(2000).into()), }; assert!(!range.matches(make_id(999))); @@ -184,21 +196,30 @@ struct RangeWithIds { } #[test] -fn clap_parses_from_and_until() { - let cmd = - RangeWithIds::try_parse_from(["test-creation-range", "--from", "3w", "--until", "1d"]) - .unwrap(); - assert!(cmd.range.from.is_some()); - assert!(cmd.range.until.is_some()); +fn clap_parses_created_since_and_created_before() { + let cmd = RangeWithIds::try_parse_from([ + "test-creation-range", + "--created-since", + "3w", + "--created-before", + "1d", + ]) + .unwrap(); + assert!(cmd.range.since.is_some()); + assert!(cmd.range.before.is_some()); assert!(cmd.range.is_set()); } #[test] fn clap_conflicts_range_with_positional_id() { let id = ConversationId::try_from(Utc.with_ymd_and_hms(2026, 1, 1, 0, 0, 0).unwrap()).unwrap(); - let err = - RangeWithIds::try_parse_from(["test-creation-range", &id.to_string(), "--from", "3w"]) - .unwrap_err(); + let err = RangeWithIds::try_parse_from([ + "test-creation-range", + &id.to_string(), + "--created-since", + "3w", + ]) + .unwrap_err(); assert!( err.to_string().contains("cannot be used with"), "expected clap conflict error, got: {err}" @@ -219,14 +240,14 @@ struct PermissiveRangeWithIds { fn clap_permissive_range_composes_with_positional_id() { // `CreationRange` is the candidate-filter variant used by `c use`. // It must *not* conflict with the positional id, so combinations like - // `jp c use ?p --from 3w` parse successfully. + // `jp c use ?p --created-since 3w` parse successfully. let id = ConversationId::try_from(Utc.with_ymd_and_hms(2026, 1, 1, 0, 0, 0).unwrap()).unwrap(); let cmd = PermissiveRangeWithIds::try_parse_from([ "test-creation-range-permissive", &id.to_string(), - "--from", + "--created-since", "3w", ]) .expect("non-exclusive range should compose with positional id"); - assert!(cmd.range.from.is_some()); + assert!(cmd.range.since.is_some()); } diff --git a/crates/jp_cli/src/cmd/turn_range.rs b/crates/jp_cli/src/cmd/turn_range.rs deleted file mode 100644 index 9af7bd2d..00000000 --- a/crates/jp_cli/src/cmd/turn_range.rs +++ /dev/null @@ -1,326 +0,0 @@ -//! 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 N` count, if set. - pub(crate) fn first(&self) -> Option { - self.first - } - - /// The `--last N` count, if set. - pub(crate) fn last(&self) -> Option { - self.last - } - - /// 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 deleted file mode 100644 index dcdaab1e..00000000 --- a/crates/jp_cli/src/cmd/turn_range_tests.rs +++ /dev/null @@ -1,172 +0,0 @@ -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_cli/src/cmd/turn_selection.rs b/crates/jp_cli/src/cmd/turn_selection.rs new file mode 100644 index 00000000..160276df --- /dev/null +++ b/crates/jp_cli/src/cmd/turn_selection.rs @@ -0,0 +1,712 @@ +//! Shared turn selection for commands that act on a subset of a conversation's +//! turns. +//! +//! [`TurnSelection`] owns the `--from`/`--to`/`--turn`/`--first`/`--last`/ +//! `--keep-first`/`--keep-last` flags and the resolution behind them, so a +//! selection built for one command means the same thing in another. +//! +//! Resolution produces a [`TurnSet`]: an ordered, non-overlapping list of +//! inclusive 0-based turn windows. +//! Most selections are a single window; `--first N --last M` produces two, +//! skipping the turns in between. +//! +//! A bound never splits a turn. +//! The two ends resolve a time value differently: `--from