diff --git a/crates/tui/src/client/chat.rs b/crates/tui/src/client/chat.rs index 1a2c4464bb..c7d8bee7bd 100644 --- a/crates/tui/src/client/chat.rs +++ b/crates/tui/src/client/chat.rs @@ -6726,6 +6726,188 @@ mod image_block_wire_tests { assert_eq!(wire[2]["tool_call_id"], "call_1"); } + #[test] + fn forkguard_restored_summary_pasted_after_tool_result_survives_restore() { + // Restore-path twin of + // `forkguard_full_summary_header_pasted_after_tool_result_is_not_relocated`: + // with a real provenance carrier earlier in the history, a later user + // turn that pastes the whole summary header must survive restore + // verbatim, and the authoritative checkpoint must land at the real + // carrier's index — not at the pasted turn's. + let pasted = + crate::compaction::build_compaction_summary_block_text("Please analyze this text", ""); + let mut messages: Vec = serde_json::from_value(serde_json::json!([ + {"role":"user","content":[{"type":"text","text":"Run the tool"}]}, + {"role":"assistant","content":[{"type":"tool_use","id":"call_1","name":"read","input":{"path":"a.txt"}}]}, + {"role":"user","content":[{"type":"tool_result","tool_use_id":"call_1","content":"done"}]} + ])) + .unwrap(); + messages.push(crate::compaction::compaction_checkpoint_message( + &crate::models::SystemPrompt::Text( + crate::compaction::build_compaction_summary_block_text("Compacted summary", ""), + ), + )); + messages.push(Message { + role: Role::User, + content: vec![ContentBlock::Text { + text: pasted.clone(), + cache_control: None, + }], + }); + let wire = build_chat_messages(None, &messages, "gpt-4o"); + let wire_before: Vec<_> = wire + .iter() + .map(|message| message["role"].as_str().unwrap()) + .collect(); + + let projected = crate::runtime_handoff::project_messages_for_restore(&messages); + let carrier_index = projected + .iter() + .position(crate::compaction::is_generated_compaction_checkpoint) + .expect("projected history keeps the provenance-stamped carrier"); + let restored = crate::compaction::restore_compaction_checkpoint( + projected, + Some(&crate::models::SystemPrompt::Text( + crate::compaction::build_compaction_summary_block_text("Compacted summary", ""), + )), + ); + + assert_eq!( + restored.len(), + messages.len(), + "restore must not drop the pasted turn: {restored:?}" + ); + assert!( + restored.iter().any(|message| { + matches!( + message.content.as_slice(), + [ContentBlock::Text { + text, + cache_control: None, + }] if text == &pasted + ) + }), + "the pasted full summary must survive restore verbatim: {restored:?}" + ); + let checkpoints: Vec = restored + .iter() + .enumerate() + .filter(|(_, message)| crate::compaction::is_generated_compaction_checkpoint(message)) + .map(|(index, _)| index) + .collect(); + assert_eq!( + checkpoints, + vec![carrier_index], + "exactly one provenance-stamped checkpoint remains, at the real carrier's index: \ + {restored:?}" + ); + + let restored_wire = build_chat_messages(None, &restored, "gpt-4o"); + let roles_after: Vec<_> = restored_wire + .iter() + .map(|message| message["role"].as_str().unwrap()) + .collect(); + assert_eq!(roles_after, wire_before, "wire roles are unchanged"); + assert_eq!( + restored_wire.last().unwrap()["content"].as_str().unwrap(), + pasted, + "the pasted turn keeps its content on the wire" + ); + } + + #[test] + fn forkguard_restored_summary_pasted_before_the_carrier_does_not_steal_the_anchor() { + // Mirror order of + // `forkguard_restored_summary_pasted_after_tool_result_survives_restore`: + // the pasted full summary precedes the real provenance carrier — the + // order in which the pre-fix loose anchor landed on the pasted turn's + // index. Restore must anchor at the real carrier's index, replace the + // carrier with the authoritative summary there, and keep the pasted + // turn verbatim. The carrier and the checkpoint argument carry + // different summary texts so the assertions cannot pass by leaving + // the original carrier untouched. + let pasted = + crate::compaction::build_compaction_summary_block_text("Please analyze this text", ""); + let mut messages: Vec = serde_json::from_value(serde_json::json!([ + {"role":"user","content":[{"type":"text","text":"Run the tool"}]}, + {"role":"assistant","content":[{"type":"tool_use","id":"call_1","name":"read","input":{"path":"a.txt"}}]}, + {"role":"user","content":[{"type":"tool_result","tool_use_id":"call_1","content":"done"}]} + ])) + .unwrap(); + messages.push(Message { + role: Role::User, + content: vec![ContentBlock::Text { + text: pasted.clone(), + cache_control: None, + }], + }); + messages.push(crate::compaction::compaction_checkpoint_message( + &crate::models::SystemPrompt::Text( + crate::compaction::build_compaction_summary_block_text("Compacted summary", ""), + ), + )); + let wire = build_chat_messages(None, &messages, "gpt-4o"); + let wire_before: Vec<_> = wire + .iter() + .map(|message| message["role"].as_str().unwrap()) + .collect(); + + let projected = crate::runtime_handoff::project_messages_for_restore(&messages); + let carrier_index = projected + .iter() + .position(crate::compaction::is_generated_compaction_checkpoint) + .expect("projected history keeps the provenance-stamped carrier"); + let authoritative = crate::models::SystemPrompt::Text( + crate::compaction::build_compaction_summary_block_text("Authoritative summary", ""), + ); + let restored = + crate::compaction::restore_compaction_checkpoint(projected, Some(&authoritative)); + + assert_eq!( + restored.len(), + messages.len(), + "restore must not drop the pasted turn: {restored:?}" + ); + assert!( + restored.iter().any(|message| { + matches!( + message.content.as_slice(), + [ContentBlock::Text { + text, + cache_control: None, + }] if text == &pasted + ) + }), + "the pasted full summary must survive restore verbatim: {restored:?}" + ); + let checkpoints: Vec = restored + .iter() + .enumerate() + .filter(|(_, message)| crate::compaction::is_generated_compaction_checkpoint(message)) + .map(|(index, _)| index) + .collect(); + assert_eq!( + checkpoints, + vec![carrier_index], + "exactly one provenance-stamped checkpoint remains, at the real carrier's index: \ + {restored:?}" + ); + assert!( + matches!( + restored[carrier_index].content.as_slice(), + [ContentBlock::Text { text, .. }, _] if text.contains("Authoritative summary") + ), + "the authoritative summary replaces the carrier at its index: {restored:?}" + ); + + let restored_wire = build_chat_messages(None, &restored, "gpt-4o"); + let roles_after: Vec<_> = restored_wire + .iter() + .map(|message| message["role"].as_str().unwrap()) + .collect(); + assert_eq!(roles_after, wire_before, "wire roles are unchanged"); + } + #[test] fn prune_only_topology_merges_with_its_prompt_on_wire() { let mut messages: Vec = serde_json::from_value(serde_json::json!([ diff --git a/crates/tui/src/compaction.rs b/crates/tui/src/compaction.rs index b5d41b77a5..fd43b8761e 100644 --- a/crates/tui/src/compaction.rs +++ b/crates/tui/src/compaction.rs @@ -160,17 +160,18 @@ by that language model. Use this to build on the work that has already been done duplicating work. Here is the summary produced by the other language model, use the information \ in this summary to assist with your own analysis:"; -/// Detection marker for committed compaction-summary text: the stable first -/// sentence of `SUMMARY_HEADER`. `engine/context.rs` restores summaries by -/// the same marker on session load. -pub const COMPACTION_SUMMARY_MARKER: &str = "Another language model started to solve this problem"; -/// Marker written by pre-v0.9.6 compaction; sessions saved under the old -/// format must still be recognized so their summary is replaced, not stacked. -pub const LEGACY_COMPACTION_SUMMARY_MARKER: &str = "Conversation Summary (Auto-Generated)"; -const COMPACTION_CHECKPOINT_PROVENANCE: &str = ""; const COMPACTION_SUMMARY_BEGIN: &str = ""; const COMPACTION_SUMMARY_END: &str = ""; +// The checkpoint-recognition family lives in the `history_recognition` leaf so +// `compaction` and `runtime_handoff` can share it without a module cycle; the +// names below stay reachable on the `compaction` path. +pub(crate) use crate::history_recognition::{ + COMPACTION_CHECKPOINT_PROVENANCE, is_compaction_checkpoint_message, + is_generated_compaction_checkpoint, +}; +pub use crate::history_recognition::{COMPACTION_SUMMARY_MARKER, LEGACY_COMPACTION_SUMMARY_MARKER}; + /// Whether a system-prompt text block is a committed compaction summary. #[must_use] pub fn is_compaction_summary_text(text: &str) -> bool { @@ -296,87 +297,36 @@ pub(crate) fn compaction_checkpoint_message(prompt: &SystemPrompt) -> Message { } } -/// Whether the first text block carries the summary header, current or legacy. -fn has_compaction_summary_header(text: &str) -> bool { - // `COMPACTION_SUMMARY_MARKER` is the current header's first sentence, and - // releases before this one also committed carriers with their own body - // after that sentence, so the prefix — not the whole header — is the - // stable shape. - text.starts_with(COMPACTION_SUMMARY_MARKER) - || text.starts_with(LEGACY_COMPACTION_SUMMARY_MARKER) -} - -/// Structural recognition of the one generated checkpoint in saved history. -/// -/// The marker substring scan above stays scoped to system-prompt carriers: -/// on history it matches an ordinary user turn that merely *quotes* the -/// header, and every consumer here either deletes or replaces what it matches. -/// Structure instead — a `role="user"` message whose first text block begins -/// with the header and whose remaining block, if any, is exactly the -/// engine-written provenance marker. -/// -/// Boundary: the single-block form has no provenance to check, because -/// releases before this one saved the bare summary, and a reload that failed -/// to recognize it would stack a second summary beside it. A user turn that -/// *begins* with the header is therefore still read as a carrier here. Request -/// rewriting does not share that reading — see -/// [`is_generated_compaction_checkpoint`] — so such a turn is never moved or -/// merged on the wire. -#[must_use] -pub(crate) fn is_compaction_checkpoint_message(message: &Message) -> bool { - let [ - ContentBlock::Text { - text, - cache_control: None, - }, - rest @ .., - ] = message.content.as_slice() - else { - return false; - }; - message.role == Role::User - && has_compaction_summary_header(text) - && (rest.is_empty() - || matches!( - rest, - [ContentBlock::Text { - text: provenance, - cache_control: None, - }] if provenance == COMPACTION_CHECKPOINT_PROVENANCE - )) -} - -/// The provenance-stamped form, and the only form request rewriting may -/// relocate or merge. A user cannot type this shape, so an ordinary turn that -/// pastes the whole summary header after a tool result keeps its position. -#[must_use] -pub(crate) fn is_generated_compaction_checkpoint(message: &Message) -> bool { - let [ - ContentBlock::Text { - cache_control: None, - .. - }, - ContentBlock::Text { - text: provenance, - cache_control: None, - }, - ] = message.content.as_slice() - else { - return false; - }; - provenance == COMPACTION_CHECKPOINT_PROVENANCE && is_compaction_checkpoint_message(message) -} - /// Replace the saved history checkpoint with the authoritative carrier, keep /// its position relative to later turns, and repair a pre-placement-fix /// Agent-topology sidecar. Both steps exist so the first request after a /// restore is wire-legal for strict paired chat templates. +/// +/// The anchor and the deletion prefer the provenance-stamped carrier: its +/// second text block is engine-written, so a pasted user turn that merely +/// starts with the summary header can neither steal the insertion position +/// nor be deleted as the carrier. The loose predicate applies only when no +/// stamped carrier exists — a structural condition, not a session age: it +/// covers saves from before the provenance block and equally histories that +/// never compacted. There a pasted header is indistinguishable from a real +/// bare carrier on content alone (or there is no carrier to anchor on at +/// all), so the historical replace-in-place applies and a pasted header can +/// still be dropped. The same loose predicate also backs the keep/recompaction +/// filters in `compaction/last_round.rs` — same recognition family, +/// explicitly out of scope here. pub(crate) fn restore_compaction_checkpoint( mut messages: Vec, checkpoint: Option<&SystemPrompt>, ) -> Vec { - let checkpoint_index = messages.iter().position(is_compaction_checkpoint_message); - messages.retain(|message| !is_compaction_checkpoint_message(message)); + let provenance_anchor = messages.iter().position(is_generated_compaction_checkpoint); + let carrier: fn(&Message) -> bool = if provenance_anchor.is_some() { + is_generated_compaction_checkpoint + } else { + is_compaction_checkpoint_message + }; + let checkpoint_index = + provenance_anchor.or_else(|| messages.iter().position(is_compaction_checkpoint_message)); + messages.retain(|message| !carrier(message)); if let Some(checkpoint) = checkpoint { let index = checkpoint_index.unwrap_or(messages.len()); messages.insert(index, compaction_checkpoint_message(checkpoint)); diff --git a/crates/tui/src/history_recognition.rs b/crates/tui/src/history_recognition.rs new file mode 100644 index 0000000000..075de30b06 --- /dev/null +++ b/crates/tui/src/history_recognition.rs @@ -0,0 +1,91 @@ +//! Structural recognition of the compaction checkpoint in saved history. +//! +//! Leaf module shared by `compaction` and `runtime_handoff`. Hosting the +//! checkpoint-recognition family here keeps `runtime_handoff` from importing +//! `compaction`: `compaction` already imports `runtime_handoff` for +//! restore-time topology relocation, so a reverse edge would close a module +//! dependency cycle. + +use crate::models::{ContentBlock, Message, Role}; + +/// Detection marker for committed compaction-summary text: the stable first +/// sentence of the summary header `compaction` commits (`SUMMARY_HEADER`). +/// `engine/context.rs` restores summaries by the same marker on session load. +pub const COMPACTION_SUMMARY_MARKER: &str = "Another language model started to solve this problem"; +/// Marker written by pre-v0.9.6 compaction; sessions saved under the old +/// format must still be recognized so their summary is replaced, not stacked. +pub const LEGACY_COMPACTION_SUMMARY_MARKER: &str = "Conversation Summary (Auto-Generated)"; +pub(crate) const COMPACTION_CHECKPOINT_PROVENANCE: &str = + ""; + +/// Whether the first text block carries the summary header, current or legacy. +fn has_compaction_summary_header(text: &str) -> bool { + // `COMPACTION_SUMMARY_MARKER` is the current header's first sentence, and + // releases before this one also committed carriers with their own body + // after that sentence, so the prefix — not the whole header — is the + // stable shape. + text.starts_with(COMPACTION_SUMMARY_MARKER) + || text.starts_with(LEGACY_COMPACTION_SUMMARY_MARKER) +} + +/// Structural recognition of the one generated checkpoint in saved history. +/// +/// The marker substring scans in `compaction` (`extract_compaction_summary`, +/// `strip_summary_text`) stay scoped to system-prompt carriers: on history +/// they match an ordinary user turn that merely *quotes* the header, and +/// every consumer here either deletes or replaces what it matches. +/// Structure instead — a `role="user"` message whose first text block begins +/// with the header and whose remaining block, if any, is exactly the +/// engine-written provenance marker. +/// +/// Boundary: the single-block form has no provenance to check, because +/// releases before this one saved the bare summary, and a reload that failed +/// to recognize it would stack a second summary beside it. A user turn that +/// *begins* with the header is therefore still read as a carrier here. Request +/// rewriting does not share that reading — see +/// [`is_generated_compaction_checkpoint`] — so such a turn is never moved or +/// merged on the wire. +#[must_use] +pub(crate) fn is_compaction_checkpoint_message(message: &Message) -> bool { + let [ + ContentBlock::Text { + text, + cache_control: None, + }, + rest @ .., + ] = message.content.as_slice() + else { + return false; + }; + message.role == Role::User + && has_compaction_summary_header(text) + && (rest.is_empty() + || matches!( + rest, + [ContentBlock::Text { + text: provenance, + cache_control: None, + }] if provenance == COMPACTION_CHECKPOINT_PROVENANCE + )) +} + +/// The provenance-stamped form, and the only form request rewriting may +/// relocate or merge. A user cannot type this shape, so an ordinary turn that +/// pastes the whole summary header after a tool result keeps its position. +#[must_use] +pub(crate) fn is_generated_compaction_checkpoint(message: &Message) -> bool { + let [ + ContentBlock::Text { + cache_control: None, + .. + }, + ContentBlock::Text { + text: provenance, + cache_control: None, + }, + ] = message.content.as_slice() + else { + return false; + }; + provenance == COMPACTION_CHECKPOINT_PROVENANCE && is_compaction_checkpoint_message(message) +} diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 99e43bf9cb..7ac675f7cc 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -70,6 +70,7 @@ pub use fleet::profile::WORKSPACE_AGENT_PROFILE_DIR; pub use fleet::roster::FleetRoster; mod goal_loop; mod hashing; +mod history_recognition; #[doc(hidden)] pub mod hooks; mod image_attach; diff --git a/crates/tui/src/runtime_handoff.rs b/crates/tui/src/runtime_handoff.rs index 2a759a07b8..f419e768bd 100644 --- a/crates/tui/src/runtime_handoff.rs +++ b/crates/tui/src/runtime_handoff.rs @@ -805,7 +805,7 @@ pub(crate) fn replace_agent_topology_checkpoint( let ends_with_tool_result = messages.last().is_some_and(carries_tool_result); let ends_with_compaction_summary = messages .last() - .is_some_and(crate::compaction::is_generated_compaction_checkpoint); + .is_some_and(crate::history_recognition::is_generated_compaction_checkpoint); let position = if ends_with_tool_result || ends_with_compaction_summary { compaction_anchor(messages, messages.len()).map_or(0, CompactionAnchor::placement_index) } else { @@ -1328,7 +1328,7 @@ fn is_compaction_topology_carrier(message: &Message) -> bool { fn restored_topology_anchor(messages: &[Message], index: usize) -> Option { let summary_before = messages[..index] .iter() - .rposition(crate::compaction::is_compaction_checkpoint_message); + .rposition(crate::history_recognition::is_compaction_checkpoint_message); let follows_tool_result = index > 0 && carries_tool_result(&messages[index - 1]); if summary_before.is_none() && !follows_tool_result { return None; @@ -1337,7 +1337,7 @@ fn restored_topology_anchor(messages: &[Message], index: usize) -> Option .or_else(|| { messages[index + 1..] .iter() - .position(crate::compaction::is_compaction_checkpoint_message) + .position(crate::history_recognition::is_compaction_checkpoint_message) .map(|offset| index + 1 + offset) }) .unwrap_or(messages.len()); @@ -1480,7 +1480,7 @@ fn is_runtime_owned_user_message(message: &Message) -> bool { // The compaction checkpoint is engine-written history, so `/edit` must // not treat it as the turn to truncate at: doing so deletes the summary // the session is built on. Structure decides, not the marker substring. - || crate::compaction::is_compaction_checkpoint_message(message) + || crate::history_recognition::is_compaction_checkpoint_message(message) } /// Return engine-owned metadata in either the current trailing shape or the