From 28e6af5c939efcecb19197113529c2da5993976c Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 29 Jun 2026 14:37:33 +0200 Subject: [PATCH] fix(cli, conversation): Show stable raw turn numbers in print headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn headers shown by `jp conversation print` now display the pre-projection (raw) turn number rather than the position in the projected iterator. This means the number in the header matches the argument `jp conversation compact --from/--to` accepts, so users can reference a printed turn directly in a compact command without having to account for how many earlier turns were collapsed. In the compacted view, a summary turn that stands in for several raw turns shows the full range it replaced, e.g. `turns 2–5, 12 minutes ago`, while a single-turn summary continues to show `turn N`. The `TurnSelection::Index` path also resolves by raw turn number now, so `jp conversation print --turn N` is consistent with the header. A new `TurnOrigin` enum is returned by `apply_projection()` and threaded through to `TurnRenderer::render_turn`, carrying the mapping from each projected turn back to its raw source turn(s). When no compaction is present every turn maps to `TurnOrigin::Kept` of its own index, so behaviour is unchanged for uncompacted views. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/conversation/print.rs | 44 +++++-- crates/jp_cli/src/render/turn.rs | 39 ++++-- crates/jp_cli/src/render/turn_tests.rs | 43 +++++++ crates/jp_conversation/src/stream.rs | 10 +- .../jp_conversation/src/stream/projection.rs | 119 +++++++++++++++++- .../src/stream/projection_tests.rs | 112 +++++++++++++++++ 6 files changed, 340 insertions(+), 27 deletions(-) create mode 100644 crates/jp_cli/src/render/turn_tests.rs diff --git a/crates/jp_cli/src/cmd/conversation/print.rs b/crates/jp_cli/src/cmd/conversation/print.rs index ea07d49a..6f36677e 100644 --- a/crates/jp_cli/src/cmd/conversation/print.rs +++ b/crates/jp_cli/src/cmd/conversation/print.rs @@ -4,7 +4,7 @@ use jp_config::{ }, style::{reasoning::ReasoningDisplayConfig, typewriter::DelayDuration}, }; -use jp_conversation::compaction::resolve_range; +use jp_conversation::{compaction::resolve_range, stream::TurnOrigin}; use jp_llm::tool::InvocationContext; use jp_workspace::ConversationHandle; @@ -140,9 +140,12 @@ impl Print { ) -> Output { let mut events = ctx.workspace.events(handle)?.clone(); - if compacted { - events.apply_projection(); - } + // Selection and the numbers shown in headers both use the raw + // (pre-projection) turn numbering, so a turn number means the same + // thing whether or not the view is compacted, and matches what + // `compact` accepts. Capture the raw count before projection collapses + // any turns. + let raw_count = events.turn_count(); let cfg = ctx.config(); let root = ctx @@ -190,17 +193,17 @@ impl Print { ); renderer.set_user_only(user_only); - let count = events.turn_count(); - - // `--last 0` explicitly selects nothing. + // `--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(count) { - return Err(format!("turn {n} out of range (conversation has {count} turns)").into()); + 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) { @@ -220,15 +223,30 @@ impl Print { Bound::At(b) => Some(b), }; - // A `from > to` or otherwise empty range selects nothing. + // 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 { renderer.flush(); return Ok(()); }; - for turn in events.iter_turns() { - if (selected.from_turn..=selected.to_turn).contains(&turn.index()) { - renderer.render_turn(&turn); + // Project for rendering when compacted; `origins` maps each rendered + // turn back to the raw turn number(s) it represents for the header. + let origins: Vec = if compacted { + events.apply_projection() + } else { + (0..raw_count).map(TurnOrigin::Kept).collect() + }; + debug_assert_eq!( + events.turn_count(), + origins.len(), + "turn origins must align with iter_turns()" + ); + + for (turn, origin) in events.iter_turns().zip(&origins) { + if origin.overlaps(selected.from_turn, selected.to_turn) { + renderer.render_turn(&turn, *origin); } } diff --git a/crates/jp_cli/src/render/turn.rs b/crates/jp_cli/src/render/turn.rs index ebd8bfd0..e16ab058 100644 --- a/crates/jp_cli/src/render/turn.rs +++ b/crates/jp_cli/src/render/turn.rs @@ -16,7 +16,10 @@ use jp_config::{ model::id::PartialModelIdOrAliasConfig, style::{StyleConfig, typewriter::DelayDuration}, }; -use jp_conversation::{EventKind, stream::turn_iter::Turn}; +use jp_conversation::{ + EventKind, + stream::{TurnOrigin, turn_iter::Turn}, +}; use jp_llm::tool::InvocationContext; use jp_printer::{ErrChannel, Printer}; use tracing::warn; @@ -106,14 +109,20 @@ impl TurnRenderer { } /// Render all events in a turn. - pub fn render_turn(&mut self, turn: &Turn<'_>) { + /// + /// `origin` maps the turn back to the raw conversation turn number(s) it + /// represents, used for the header. + /// For a non-projected stream it is [`TurnOrigin::Kept`] of the turn's own + /// index; for a compacted view a summary turn carries the range it + /// replaced. + pub fn render_turn(&mut self, turn: &Turn<'_>, origin: TurnOrigin) { if matches!(self.source, ConfigSource::PerTurn) && let Some(partial) = turn.iter().next().map(|e| &e.config) { self.reconfigure(partial); } - self.view.set_turn_detail(turn_detail(turn)); + self.view.set_turn_detail(turn_detail(turn, origin)); for event_with_cfg in turn { if self.user_only @@ -226,13 +235,18 @@ impl TurnRenderer { } } -/// Build the dimmed right-aligned header detail for a turn: its 1-based number -/// and how long ago it started, e.g. `turn 2, 12 minutes ago`. +/// Build the dimmed right-aligned header detail for a turn: its raw turn +/// number(s) and how long ago it started, e.g. `turn 2, 12 minutes ago` or, for +/// a compaction summary spanning several turns, `turns 2–5, 12 minutes ago`. +/// +/// The number comes from `origin` (the raw turn number `jp conversation +/// compact` accepts), not the turn's position in the iterator, so it stays +/// stable whether or not the view is compacted. /// /// The timestamp comes from the turn's `TurnStart` marker, falling back to the /// turn's first event when the marker is absent (the implicit leading turn of a /// legacy stream). -fn turn_detail(turn: &Turn<'_>) -> Option { +fn turn_detail(turn: &Turn<'_>, origin: TurnOrigin) -> Option { let started_at = turn .iter() .find(|e| e.event.is_turn_start()) @@ -244,7 +258,14 @@ fn turn_detail(turn: &Turn<'_>) -> Option { // renders as "now" rather than a misleading "... ago". let elapsed = (Utc::now() - started_at).to_std().unwrap_or_default(); let ago = timeago::Formatter::new().convert(elapsed); - Some(format!("turn {}, {ago}", turn.index() + 1)) + + let number = match origin { + TurnOrigin::Kept(index) => format!("turn {}", index + 1), + TurnOrigin::Summary { from, to } if from == to => format!("turn {}", from + 1), + TurnOrigin::Summary { from, to } => format!("turns {}\u{2013}{}", from + 1, to + 1), + }; + + Some(format!("{number}, {ago}")) } /// Render a partial model id as a display string, treating a fully-empty id as @@ -258,3 +279,7 @@ fn render_model_id(id: &PartialModelIdOrAliasConfig) -> Option { let s = id.to_string(); if s.is_empty() { None } else { Some(s) } } + +#[cfg(test)] +#[path = "turn_tests.rs"] +mod tests; diff --git a/crates/jp_cli/src/render/turn_tests.rs b/crates/jp_cli/src/render/turn_tests.rs new file mode 100644 index 00000000..71c4bba0 --- /dev/null +++ b/crates/jp_cli/src/render/turn_tests.rs @@ -0,0 +1,43 @@ +use jp_conversation::{ConversationStream, event::ChatRequest}; + +use super::{TurnOrigin, turn_detail}; + +/// A single-turn stream whose turn carries a timestamp, so `turn_detail` +/// produces `Some`. +/// The "ago" suffix is time-dependent, so assertions only check the turn-number +/// prefix. +fn single_turn() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.start_turn(ChatRequest::from("hello")); + stream +} + +#[test] +fn kept_turn_shows_its_raw_number() { + let stream = single_turn(); + let turn = stream.iter_turns().next().unwrap(); + + let detail = turn_detail(&turn, TurnOrigin::Kept(5)).unwrap(); + + assert!(detail.starts_with("turn 6, "), "got: {detail}"); +} + +#[test] +fn multi_turn_summary_shows_the_collapsed_range() { + let stream = single_turn(); + let turn = stream.iter_turns().next().unwrap(); + + let detail = turn_detail(&turn, TurnOrigin::Summary { from: 1, to: 4 }).unwrap(); + + assert!(detail.starts_with("turns 2\u{2013}5, "), "got: {detail}"); +} + +#[test] +fn single_turn_summary_shows_one_number() { + let stream = single_turn(); + let turn = stream.iter_turns().next().unwrap(); + + let detail = turn_detail(&turn, TurnOrigin::Summary { from: 2, to: 2 }).unwrap(); + + assert!(detail.starts_with("turn 3, "), "got: {detail}"); +} diff --git a/crates/jp_conversation/src/stream.rs b/crates/jp_conversation/src/stream.rs index 05e0a0e5..19863af4 100644 --- a/crates/jp_conversation/src/stream.rs +++ b/crates/jp_conversation/src/stream.rs @@ -11,6 +11,7 @@ use tracing::{error, warn}; mod projection; pub mod turn_iter; pub mod turn_mut; +pub use projection::TurnOrigin; pub use turn_iter::{IterTurns, Turn}; pub use turn_mut::TurnMut; @@ -437,14 +438,17 @@ impl ConversationStream { /// After this call, the stream's conversation events represent what the LLM /// should see. /// - /// This is a no-op when no compaction events are present. + /// Returns one [`TurnOrigin`] per resulting turn, in turn order, mapping + /// each projected turn back to the raw turn number(s) it represents. + /// When no compaction events are present the events are left unchanged and + /// every turn maps to its own index. /// /// This method is called by [`Thread::into_parts()`] before provider /// visibility filtering. /// /// [`Thread::into_parts()`]: crate::thread::Thread::into_parts - pub fn apply_projection(&mut self) { - projection::apply(&mut self.events); + pub fn apply_projection(&mut self) -> Vec { + projection::apply(&mut self.events) } /// Start a new turn with the given chat request. diff --git a/crates/jp_conversation/src/stream/projection.rs b/crates/jp_conversation/src/stream/projection.rs index 11c4e9c9..d3e2434e 100644 --- a/crates/jp_conversation/src/stream/projection.rs +++ b/crates/jp_conversation/src/stream/projection.rs @@ -16,6 +16,46 @@ use crate::{ event::{ChatRequest, ChatResponse, ConversationEvent, TurnStart}, }; +/// Which raw conversation turn(s) a projected turn stands for. +/// +/// Turn numbering from [`IterTurns`] is positional, which matches the raw +/// stream but shifts once projection collapses a summarized range into one +/// synthetic turn. +/// [`apply`] returns one `TurnOrigin` per resulting turn so callers can display +/// the original (pre-projection) turn numbers. +/// +/// [`IterTurns`]: super::IterTurns +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TurnOrigin { + /// A turn carried through projection unchanged, at this 0-based raw turn + /// index. + Kept(usize), + /// A synthetic summary turn replacing the raw turns `from..=to` (0-based, + /// inclusive). + Summary { + /// First raw turn the summary replaces. + from: usize, + /// Last raw turn the summary replaces. + to: usize, + }, +} + +impl TurnOrigin { + /// Whether this projected turn represents any raw turn in `from..=to` + /// (0-based, inclusive). + /// + /// Lets a selection resolved against raw turn numbers pick the projected + /// turns to render, including a summary turn that stands in for part of the + /// range. + #[must_use] + pub const fn overlaps(&self, from: usize, to: usize) -> bool { + match *self { + Self::Kept(index) => from <= index && index <= to, + Self::Summary { from: f, to: t } => f <= to && from <= t, + } + } +} + /// Resolved compaction policies for a single turn. struct TurnPolicy { /// Summary covering this turn. @@ -62,8 +102,11 @@ struct ResolvedSummary { /// content with a status line. /// - **Tool call omit**: removes tool call request/response pairs. /// +/// Returns one [`TurnOrigin`] per resulting turn, in turn order, mapping each +/// projected turn back to the raw turn number(s) it represents. +/// /// [`Compaction`]: crate::Compaction -pub(super) fn apply(events: &mut Vec) { +pub(super) fn apply(events: &mut Vec) -> Vec { let compactions: Vec<_> = events .iter() .filter_map(|e| match e { @@ -73,7 +116,12 @@ pub(super) fn apply(events: &mut Vec) { .collect(); if compactions.is_empty() { - return; + // Nothing collapses, so every turn maps to itself. + let event_origins: Vec = assign_turn_indices(events) + .into_iter() + .map(TurnOrigin::Kept) + .collect(); + return collect_turn_origins(events, &event_origins); } let turn_indices = assign_turn_indices(events); @@ -96,6 +144,9 @@ pub(super) fn apply(events: &mut Vec) { .collect(); let mut projected = Vec::with_capacity(events.len()); + // Raw origin of each projected event, kept in lockstep with `projected` so + // turn numbering can recover the pre-projection turn numbers. + let mut event_origins: Vec = Vec::with_capacity(events.len()); let mut summaries_injected: HashSet = HashSet::new(); for (i, event) in std::mem::take(events).into_iter().enumerate() { @@ -107,6 +158,7 @@ pub(super) fn apply(events: &mut Vec) { // skip unknown events, so they stay invisible to providers. InternalEvent::ConfigDelta(_) | InternalEvent::Unknown(_) => { projected.push(event); + event_origins.push(TurnOrigin::Kept(turn)); } // Compaction events are consumed by projection — they've been // applied and should not survive into the projected stream. @@ -114,13 +166,32 @@ pub(super) fn apply(events: &mut Vec) { InternalEvent::Event(conv_event) => { let Some(policy) = policies.get(turn) else { projected.push(InternalEvent::Event(conv_event)); + event_origins.push(TurnOrigin::Kept(turn)); continue; }; // Summary takes precedence over all per-type policies. if let Some(summary) = &policy.summary { if inject_at_turn.contains(&turn) && summaries_injected.insert(turn) { - inject_summary(&mut projected, &summary.text, conv_event.timestamp); + // The injected summary stands in for the contiguous run + // of raw turns that resolve to this same summary — that + // is what visually collapses into one turn. Display the + // run, not the compaction's declared range, which a + // newer fully-contained summary can split in two. + let mut run_to = turn; + while run_to + 1 < policies.len() + && policies[run_to + 1].summary.as_ref() == Some(summary) + { + run_to += 1; + } + inject_summary( + &mut projected, + &mut event_origins, + &summary.text, + conv_event.timestamp, + turn, + run_to, + ); } // Drop the original event — it's covered by the summary. continue; @@ -157,11 +228,36 @@ pub(super) fn apply(events: &mut Vec) { } projected.push(InternalEvent::Event(Box::new(event))); + event_origins.push(TurnOrigin::Kept(turn)); } } } *events = projected; + collect_turn_origins(events, &event_origins) +} + +/// Group projected events into turns (matching [`IterTurns`]) and return each +/// turn's [`TurnOrigin`], read from the event that opens the turn. +/// +/// `event_origins` must be in lockstep with `events`. +/// +/// [`IterTurns`]: super::IterTurns +fn collect_turn_origins(events: &[InternalEvent], event_origins: &[TurnOrigin]) -> Vec { + let mut origins = Vec::new(); + let mut current_has_event = false; + for (i, event) in events.iter().enumerate() { + let Some(conv_event) = event.as_event() else { + continue; + }; + // A turn opens at the first event and at every later `TurnStart`, + // mirroring `IterTurns`' boundary rule exactly. + if !current_has_event || conv_event.is_turn_start() { + origins.push(event_origins[i]); + } + current_has_event = true; + } + origins } /// Assign a 0-based turn index to each event position. @@ -270,18 +366,33 @@ fn resolve_policies(max_turn: usize, compactions: &[crate::Compaction]) -> Vec, summary: &str, timestamp: DateTime) { +/// +/// `from`/`to` are the raw turn range this summary replaces; every injected +/// event records it as its [`TurnOrigin`] so the run stays in lockstep with +/// `events`. +fn inject_summary( + events: &mut Vec, + origins: &mut Vec, + summary: &str, + timestamp: DateTime, + from: usize, + to: usize, +) { + let origin = TurnOrigin::Summary { from, to }; events.push(InternalEvent::Event(Box::new(ConversationEvent::new( TurnStart, timestamp, )))); + origins.push(origin); events.push(InternalEvent::Event(Box::new(ConversationEvent::new( ChatRequest::from("[Summary of previous conversation]"), timestamp, )))); + origins.push(origin); events.push(InternalEvent::Event(Box::new(ConversationEvent::new( ChatResponse::message(summary), timestamp, )))); + origins.push(origin); } /// Blank a tool call request's arguments. diff --git a/crates/jp_conversation/src/stream/projection_tests.rs b/crates/jp_conversation/src/stream/projection_tests.rs index 873212cb..50ecc499 100644 --- a/crates/jp_conversation/src/stream/projection_tests.rs +++ b/crates/jp_conversation/src/stream/projection_tests.rs @@ -8,6 +8,7 @@ use crate::{ Compaction, ConversationEvent, ConversationStream, EventKind, ReasoningPolicy, SummaryPolicy, ToolCallPolicy, event::{ChatRequest, ChatResponse, ToolCallRequest, ToolCallResponse, TurnStart}, + stream::TurnOrigin, }; // --------------------------------------------------------------------------- @@ -89,6 +90,23 @@ fn two_turn_stream() -> ConversationStream { stream } +/// Build a stream of `count` single-message turns (`q0`/`m0`, `q1`/`m1`, ...). +fn message_turns(count: usize) -> ConversationStream { + let mut stream = ConversationStream::new_test(); + for t in 0..count { + stream.push(ConversationEvent::new(TurnStart, ts(0))); + stream.push(ConversationEvent::new( + ChatRequest::from(format!("q{t}")), + ts(0), + )); + stream.push(ConversationEvent::new( + ChatResponse::message(format!("m{t}")), + ts(0), + )); + } + stream +} + /// Collect only provider-visible events from the stream (what providers see). fn visible_events(stream: &ConversationStream) -> Vec<&EventKind> { stream @@ -112,6 +130,100 @@ fn no_compaction_is_noop() { assert_eq!(stream.len(), len_before); } +// --------------------------------------------------------------------------- +// Turn origins (raw turn numbering through projection) +// --------------------------------------------------------------------------- + +#[test] +fn no_compaction_returns_kept_origins() { + let mut stream = two_turn_stream(); + + let origins = stream.apply_projection(); + + assert_eq!(origins, vec![TurnOrigin::Kept(0), TurnOrigin::Kept(1)]); +} + +#[test] +fn summary_origins_preserve_raw_turn_numbers() { + // Six turns; summarize the middle four (raw turns 1..=4). The turn after + // the summary must keep its raw number (5) rather than shifting down, so + // `jp conversation compact --from` numbers stay valid in the printed view. + let mut stream = message_turns(6); + stream.add_compaction(Compaction { + timestamp: ts(2), + from_turn: 1, + to_turn: 4, + summary: Some(SummaryPolicy { + summary: "middle turns".into(), + }), + reasoning: None, + tool_calls: None, + }); + + let origins = stream.apply_projection(); + + assert_eq!(origins, vec![ + TurnOrigin::Kept(0), + TurnOrigin::Summary { from: 1, to: 4 }, + TurnOrigin::Kept(5), + ]); +} + +#[test] +fn mechanical_compaction_keeps_one_to_one_origins() { + // Stripping reasoning/tool calls does not collapse turns, so numbering is + // unchanged. + let mut stream = two_turn_stream(); + stream.add_compaction(Compaction { + timestamp: ts(2), + from_turn: 0, + to_turn: 1, + summary: None, + reasoning: Some(ReasoningPolicy::Strip), + tool_calls: None, + }); + + let origins = stream.apply_projection(); + + assert_eq!(origins, vec![TurnOrigin::Kept(0), TurnOrigin::Kept(1)]); +} + +#[test] +fn contained_summary_origins_reflect_actual_runs() { + // OUTER over turns 0..=3, then a newer INNER over 1..=2 splits it. Each + // injected summary turn reports the contiguous run it actually collapses, + // not OUTER's declared 0..=3 range. + let mut stream = message_turns(4); + stream.add_compaction(Compaction { + timestamp: ts(1), + from_turn: 0, + to_turn: 3, + summary: Some(SummaryPolicy { + summary: "OUTER".into(), + }), + reasoning: None, + tool_calls: None, + }); + stream.add_compaction(Compaction { + timestamp: ts(2), + from_turn: 1, + to_turn: 2, + summary: Some(SummaryPolicy { + summary: "INNER".into(), + }), + reasoning: None, + tool_calls: None, + }); + + let origins = stream.apply_projection(); + + assert_eq!(origins, vec![ + TurnOrigin::Summary { from: 0, to: 0 }, + TurnOrigin::Summary { from: 1, to: 2 }, + TurnOrigin::Summary { from: 3, to: 3 }, + ]); +} + // --------------------------------------------------------------------------- // Reasoning strip // ---------------------------------------------------------------------------