From b1ddeb112899c691e5593cdd638972819037b268 Mon Sep 17 00:00:00 2001 From: Hassan Date: Tue, 7 Oct 2025 10:01:07 -0400 Subject: [PATCH 1/2] feat(tangent): Add compact subcommand with in-place summary replacement - Add /tangent compact subcommand to compact tangent conversations - Replace tangent entries with single summary entry in history - Preserve chronological order with summaries at tangent positions - Support multiple tangent compacts without losing previous summaries - Add comprehensive tests for single and multiple tangent compacts Fixes issue where tangent compact summaries were lost when exiting tangent mode. Summary now appears as history entry at exact position where tangent occurred, maintaining perfect chronological order. https://issues.amazon.com/issues/P129406383 --- crates/chat-cli/src/cli/chat/cli/tangent.rs | 252 +++++++++++++++++++ crates/chat-cli/src/cli/chat/conversation.rs | 88 ++++++- 2 files changed, 337 insertions(+), 3 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/cli/tangent.rs b/crates/chat-cli/src/cli/chat/cli/tangent.rs index fd54238c4..b6286e972 100644 --- a/crates/chat-cli/src/cli/chat/cli/tangent.rs +++ b/crates/chat-cli/src/cli/chat/cli/tangent.rs @@ -19,6 +19,8 @@ use crate::cli::experiment::experiment_manager::{ }; use crate::os::Os; +use super::compact::CompactArgs; + #[derive(Debug, PartialEq, Args)] pub struct TangentArgs { #[command(subcommand)] @@ -29,6 +31,8 @@ pub struct TangentArgs { pub enum TangentSubcommand { /// Exit tangent mode and keep the last conversation entry (user question + assistant response) Tail, + /// Compact tangent conversation and return to main session with summary + Compact(CompactArgs), } impl TangentArgs { @@ -62,6 +66,84 @@ impl TangentArgs { } match self.subcommand { + Some(TangentSubcommand::Compact(compact_args)) => { + if !session.conversation.is_in_tangent_mode() { + execute!( + session.stderr, + style::SetForegroundColor(Color::Red), + style::Print("You need to be in tangent mode to use compact.\n"), + style::SetForegroundColor(Color::Reset) + )?; + return Ok(ChatState::PromptUser { + skip_printing_tools: true, + }); + } + + // If tangent conversation is empty, just exit + if session.conversation.is_tangent_empty() { + let duration_seconds = session.conversation.get_tangent_duration_seconds().unwrap_or(0); + session.conversation.exit_tangent_mode(); + Self::send_tangent_telemetry(os, session, duration_seconds).await; + + execute!( + session.stderr, + style::SetForegroundColor(Color::DarkGrey), + style::Print("Tangent conversation was empty. Restored conversation from checkpoint ("), + style::SetForegroundColor(Color::Yellow), + style::Print("↯"), + style::SetForegroundColor(Color::DarkGrey), + style::Print(").\n"), + style::SetForegroundColor(Color::Reset) + )?; + return Ok(ChatState::PromptUser { + skip_printing_tools: true, + }); + } + + // Execute compact on the tangent conversation with error handling + let duration_seconds = session.conversation.get_tangent_duration_seconds().unwrap_or(0); + let result = compact_args.execute(&mut os.clone(), session).await; + + // Handle result - exit tangent mode regardless of success/failure + match result { + Ok(state) => { + // Success: exit tangent mode with the summary + session.conversation.exit_tangent_mode_with_compact(); + Self::send_tangent_telemetry(os, session, duration_seconds).await; + + execute!( + session.stderr, + style::SetForegroundColor(Color::DarkGrey), + style::Print("Restored conversation from checkpoint ("), + style::SetForegroundColor(Color::Yellow), + style::Print("↯"), + style::SetForegroundColor(Color::DarkGrey), + style::Print(") with tangent summary preserved.\n"), + style::SetForegroundColor(Color::Reset) + )?; + + return Ok(state); + }, + Err(err) => { + // Error: exit tangent mode without preserving anything + session.conversation.exit_tangent_mode(); + Self::send_tangent_telemetry(os, session, duration_seconds).await; + + execute!( + session.stderr, + style::SetForegroundColor(Color::Yellow), + style::Print("Compact failed. Restored conversation from checkpoint ("), + style::SetForegroundColor(Color::Yellow), + style::Print("↯"), + style::SetForegroundColor(Color::Yellow), + style::Print(") without changes.\n"), + style::SetForegroundColor(Color::Reset) + )?; + + return Err(err); + } + } + }, Some(TangentSubcommand::Tail) => { // Check if checkpoint is enabled if ExperimentManager::is_enabled(os, ExperimentName::Checkpoint) { @@ -209,4 +291,174 @@ mod tests { assert!(!conversation.is_in_tangent_mode()); assert!(conversation.get_tangent_duration_seconds().is_none()); } + + #[tokio::test] + async fn test_exit_tangent_mode_with_compact() { + use crate::cli::chat::message::{AssistantMessage, UserMessage}; + use crate::cli::chat::RequestMetadata; + + let mut os = Os::new().await.unwrap(); + let agents = Agents::default(); + let mut tool_manager = ToolManager::default(); + let mut conversation = ConversationState::new( + "test_conv_id", + agents, + tool_manager.load_tools(&mut os, &mut vec![]).await.unwrap(), + tool_manager, + None, + &os, + false, + ) + .await; + + // Add some history to main conversation + let main_user = UserMessage::new_prompt("Main question".to_string(), None); + let main_assistant = AssistantMessage::new_response(None, "Main answer".to_string()); + conversation.append_to_history_for_test(main_user, main_assistant, None); + + let main_history_len = conversation.history().len(); + assert_eq!(main_history_len, 1, "Main conversation should have 1 entry"); + + // Enter tangent mode + conversation.enter_tangent_mode(); + assert!(conversation.is_in_tangent_mode()); + + // Add tangent conversation + let tangent_user = UserMessage::new_prompt("Tangent question".to_string(), None); + let tangent_assistant = AssistantMessage::new_response(None, "Tangent answer".to_string()); + conversation.append_to_history_for_test(tangent_user, tangent_assistant, None); + + // Simulate compact creating a summary + let summary_text = "Summary of tangent conversation".to_string(); + let summary_metadata = RequestMetadata { + request_id: Some("test_request".to_string()), + message_id: "test_message".to_string(), + ..Default::default() + }; + conversation.set_latest_summary_for_test(summary_text.clone(), summary_metadata.clone()); + + let tangent_history_len = conversation.history().len(); + assert_eq!(tangent_history_len, 2, "Tangent should have added 1 entry (total 2)"); + + // Exit with compact + conversation.exit_tangent_mode_with_compact(); + assert!(!conversation.is_in_tangent_mode()); + + // Verify main conversation was restored WITH summary entry added + assert_eq!( + conversation.history().len(), + main_history_len + 1, + "Main conversation should have original entries plus summary entry" + ); + + // Verify the last entry is the summary entry + let last_entry = conversation.history().back().unwrap(); + assert_eq!( + last_entry.user.prompt(), + Some("[Tangent conversation]"), + "Summary entry user message should be tangent marker" + ); + assert!( + last_entry.assistant.content().contains(&summary_text), + "Summary entry assistant message should contain the summary" + ); + + // Verify latest_summary was NOT set (summary is in history instead) + let summary_info = conversation.get_latest_summary_for_test(); + assert!( + summary_info.is_none(), + "latest_summary should be None (summary is in history)" + ); + } + + #[tokio::test] + async fn test_multiple_tangent_compacts() { + use crate::cli::chat::message::{AssistantMessage, UserMessage}; + use crate::cli::chat::RequestMetadata; + + let mut os = Os::new().await.unwrap(); + let agents = Agents::default(); + let mut tool_manager = ToolManager::default(); + let mut conversation = ConversationState::new( + "test_conv_id", + agents, + tool_manager.load_tools(&mut os, &mut vec![]).await.unwrap(), + tool_manager, + None, + &os, + false, + ) + .await; + + // Add main conversation + conversation.append_to_history_for_test( + UserMessage::new_prompt("Main Q1".to_string(), None), + AssistantMessage::new_response(None, "Main A1".to_string()), + None, + ); + + // First tangent + conversation.enter_tangent_mode(); + conversation.append_to_history_for_test( + UserMessage::new_prompt("Tangent1 Q".to_string(), None), + AssistantMessage::new_response(None, "Tangent1 A".to_string()), + None, + ); + conversation.set_latest_summary_for_test( + "Summary of tangent 1".to_string(), + RequestMetadata { + request_id: Some("req1".to_string()), + message_id: "msg1".to_string(), + ..Default::default() + }, + ); + conversation.exit_tangent_mode_with_compact(); + + assert_eq!(conversation.history().len(), 2, "Should have main + tangent1 summary"); + assert_eq!( + conversation.history().back().unwrap().user.prompt(), + Some("[Tangent conversation]"), + "Last entry should be tangent1 summary" + ); + + // Add more main conversation + conversation.append_to_history_for_test( + UserMessage::new_prompt("Main Q2".to_string(), None), + AssistantMessage::new_response(None, "Main A2".to_string()), + None, + ); + + // Second tangent + conversation.enter_tangent_mode(); + conversation.append_to_history_for_test( + UserMessage::new_prompt("Tangent2 Q".to_string(), None), + AssistantMessage::new_response(None, "Tangent2 A".to_string()), + None, + ); + conversation.set_latest_summary_for_test( + "Summary of tangent 2".to_string(), + RequestMetadata { + request_id: Some("req2".to_string()), + message_id: "msg2".to_string(), + ..Default::default() + }, + ); + conversation.exit_tangent_mode_with_compact(); + + // Verify final state + assert_eq!( + conversation.history().len(), + 4, + "Should have: main1, tangent1_summary, main2, tangent2_summary" + ); + + // Verify chronological order + let history: Vec<_> = conversation.history().iter().collect(); + assert_eq!(history[0].user.prompt(), Some("Main Q1")); + assert_eq!(history[1].user.prompt(), Some("[Tangent conversation]")); + assert!(history[1].assistant.content().contains("Summary of tangent 1")); + assert_eq!(history[2].user.prompt(), Some("Main Q2")); + assert_eq!(history[3].user.prompt(), Some("[Tangent conversation]")); + assert!(history[3].assistant.content().contains("Summary of tangent 2")); + } } diff --git a/crates/chat-cli/src/cli/chat/conversation.rs b/crates/chat-cli/src/cli/chat/conversation.rs index 022e42e74..08941b7f5 100644 --- a/crates/chat-cli/src/cli/chat/conversation.rs +++ b/crates/chat-cli/src/cli/chat/conversation.rs @@ -90,10 +90,10 @@ pub const CONTEXT_ENTRY_END_HEADER: &str = "--- CONTEXT ENTRY END ---\n\n"; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct HistoryEntry { - user: UserMessage, - assistant: AssistantMessage, + pub user: UserMessage, + pub assistant: AssistantMessage, #[serde(default)] - request_metadata: Option, + pub request_metadata: Option, } #[derive(Debug, Clone)] @@ -235,6 +235,15 @@ impl ConversationState { self.tangent_state.is_some() } + /// Check if tangent conversation is empty (no new messages since entering tangent mode) + pub fn is_tangent_empty(&self) -> bool { + if let Some(checkpoint) = &self.tangent_state { + self.history.len() <= checkpoint.main_history.len() + } else { + false + } + } + /// Create a checkpoint of current conversation state fn create_checkpoint(&self) -> ConversationCheckpoint { ConversationCheckpoint { @@ -302,6 +311,50 @@ impl ConversationState { } } + /// Exit tangent mode with compact - called after compact completes + /// Replaces tangent entries with a single summary entry in-place + pub fn exit_tangent_mode_with_compact(&mut self) { + if let Some(checkpoint) = self.tangent_state.take() { + tracing::debug!( + "exit_tangent_mode_with_compact: tangent history len={}, latest_summary present={}", + self.history.len(), + self.latest_summary.is_some() + ); + + // Capture the tangent's latest_summary (created by compact) + let tangent_summary = self.latest_summary.clone(); + + // Restore main history + self.history = checkpoint.main_history; + self.next_message = checkpoint.main_next_message; + self.transcript = checkpoint.main_transcript; + self.latest_summary = checkpoint.main_latest_summary; + self.valid_history_range = (0, self.history.len()); + + tracing::debug!( + "exit_tangent_mode_with_compact: after restore, main history len={}", + self.history.len() + ); + + // Add summary as a history entry at the tangent position + if let Some((summary, metadata)) = tangent_summary { + tracing::debug!("exit_tangent_mode_with_compact: adding tangent summary as history entry"); + let summary_entry = HistoryEntry { + user: UserMessage::new_prompt("[Tangent conversation]".to_string(), None), + assistant: AssistantMessage::new_response(None, summary), + request_metadata: Some(metadata), + }; + self.history.push_back(summary_entry); + self.valid_history_range = (0, self.history.len()); + } + + if let Some(manager) = self.checkpoint_manager.as_mut() { + manager.message_locked = false; + manager.pending_user_message = None; + } + } + } + /// Appends a collection prompts into history and returns the last message in the collection. /// It asserts that the collection ends with a prompt that assumes the role of user. pub fn append_prompts(&mut self, mut prompts: VecDeque) -> Option { @@ -1260,6 +1313,35 @@ fn enforce_tool_use_history_invariants(history: &mut VecDeque, too } } +impl ConversationState { + /// Test helper: Append a history entry directly (bypasses normal flow) + #[cfg(test)] + pub fn append_to_history_for_test( + &mut self, + user: UserMessage, + assistant: AssistantMessage, + request_metadata: Option, + ) { + self.history.push_back(HistoryEntry { + user, + assistant, + request_metadata, + }); + } + + /// Test helper: Set latest summary directly + #[cfg(test)] + pub fn set_latest_summary_for_test(&mut self, summary: String, metadata: RequestMetadata) { + self.latest_summary = Some((summary, metadata)); + } + + /// Test helper: Get latest summary + #[cfg(test)] + pub fn get_latest_summary_for_test(&self) -> Option<(String, RequestMetadata)> { + self.latest_summary.clone() + } +} + fn default_true() -> bool { true } From 519c0263287195619a4ec1ca7c1418d2519e3a4a Mon Sep 17 00:00:00 2001 From: Hassan Date: Wed, 8 Oct 2025 12:28:05 -0400 Subject: [PATCH 2/2] chore: Remove debug tracing statements from tangent compact --- crates/chat-cli/src/cli/chat/conversation.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/conversation.rs b/crates/chat-cli/src/cli/chat/conversation.rs index 08941b7f5..4edff93d9 100644 --- a/crates/chat-cli/src/cli/chat/conversation.rs +++ b/crates/chat-cli/src/cli/chat/conversation.rs @@ -315,12 +315,6 @@ impl ConversationState { /// Replaces tangent entries with a single summary entry in-place pub fn exit_tangent_mode_with_compact(&mut self) { if let Some(checkpoint) = self.tangent_state.take() { - tracing::debug!( - "exit_tangent_mode_with_compact: tangent history len={}, latest_summary present={}", - self.history.len(), - self.latest_summary.is_some() - ); - // Capture the tangent's latest_summary (created by compact) let tangent_summary = self.latest_summary.clone(); @@ -330,15 +324,9 @@ impl ConversationState { self.transcript = checkpoint.main_transcript; self.latest_summary = checkpoint.main_latest_summary; self.valid_history_range = (0, self.history.len()); - - tracing::debug!( - "exit_tangent_mode_with_compact: after restore, main history len={}", - self.history.len() - ); // Add summary as a history entry at the tangent position if let Some((summary, metadata)) = tangent_summary { - tracing::debug!("exit_tangent_mode_with_compact: adding tangent summary as history entry"); let summary_entry = HistoryEntry { user: UserMessage::new_prompt("[Tangent conversation]".to_string(), None), assistant: AssistantMessage::new_response(None, summary),