From 85e8a5b1ed6dfd38a0970ddf273a5ddd47bf3c63 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 19:13:09 +0530 Subject: [PATCH 01/18] feat: add interactive :rewind command to truncate conversations Implements an interactive rewind command that allows users to roll back a conversation to any earlier message, discarding all subsequent messages, tool results, and usage data. Changes: - crates/forge_domain/src/context.rs: Add Context::truncate() and Context::format_messages_for_rewind() methods - crates/forge_main/src/model.rs: Add Rewind variant to AppCommand enum - crates/forge_main/src/ui.rs: Add on_slash_rewind handler with interactive conversation picker, message list display, and prompt for target index Usage: :rewind [conversation-id] - With an ID: rewinds the specified conversation - Without an ID: rewinds the active conversation, or shows a picker - Displays indexed message previews and prompts for truncation point - Saves the truncated conversation and shows confirmation --- crates/forge_domain/src/context.rs | 39 +++++++++ crates/forge_main/src/model.rs | 10 +++ crates/forge_main/src/ui.rs | 136 +++++++++++++++++++++++++++++ 3 files changed, 185 insertions(+) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index c2f0f30fde..f056802cdb 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -685,6 +685,45 @@ impl Context { self.messages.len() } + /// Truncates the context, keeping messages up to and including the given + /// index, and removing all messages after that point. + /// + /// Also removes tool results that reference tool calls that were + /// truncated, and resets usage data to match the remaining messages. + /// + /// Returns the truncated context. + pub fn truncate(mut self, keep_up_to: usize) -> Self { + if keep_up_to >= self.messages.len() { + return self; + } + self.messages.truncate(keep_up_to + 1); + self + } + + /// Formats messages with numbered indices for display during interactive + /// rewind. Each line shows: `[N] Role: preview_content` + pub fn format_messages_for_rewind(&self) -> Vec { + self.messages + .iter() + .enumerate() + .map(|(i, entry)| { + let role = match &**entry { + ContextMessage::Text(msg) => format!("{:?}", msg.role), + ContextMessage::Tool(_) => "Tool".to_string(), + ContextMessage::Image(_) => "Image".to_string(), + }; + let preview = entry.content().unwrap_or("").trim(); + let max_len = 100; + let preview = if preview.len() > max_len { + format!("{}...", &preview[..max_len]) + } else { + preview.to_string() + }; + format!("[{i:>3}] {role:>9}: {preview}") + }) + .collect() + } + /// Returns the count of user messages in the context pub fn user_message_count(&self) -> usize { self.messages diff --git a/crates/forge_main/src/model.rs b/crates/forge_main/src/model.rs index 5ee93c0405..94913b7d29 100644 --- a/crates/forge_main/src/model.rs +++ b/crates/forge_main/src/model.rs @@ -494,6 +494,15 @@ pub enum AppCommand { id: Option, }, + /// Rewind a conversation to an earlier message, discarding subsequent + /// messages. This can be triggered with the '/rewind' command. + #[strum(props(usage = "Rewind a conversation to an earlier message"))] + Rewind { + /// Conversation ID to rewind (optional — prompts interactively if + /// absent) + id: Option, + }, + /// Rename any conversation interactively. /// This can be triggered with the '/conversation-rename' command. #[strum(props(usage = "Rename a conversation interactively"))] @@ -733,6 +742,7 @@ impl AppCommand { AppCommand::CommitPreview => "commit-preview", AppCommand::Suggest { .. } => "suggest", AppCommand::Clone { .. } => "clone", + AppCommand::Rewind { .. } => "rewind", AppCommand::ConversationRename { .. } => "conversation-rename", AppCommand::Copy => "copy", AppCommand::WorkspaceSync => "workspace-sync", diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 7d2034f26d..b95b262394 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2271,6 +2271,9 @@ impl A + Send + Sync> UI AppCommand::Clone { id } => { self.on_slash_clone(id).await?; } + AppCommand::Rewind { id } => { + self.on_slash_rewind(id).await?; + } AppCommand::ConversationRename { name } => { let args = if name.is_empty() { None @@ -2622,6 +2625,139 @@ impl A + Send + Sync> UI Ok(()) } + /// Rewinds a conversation to an earlier message, discarding all subsequent + /// messages and their associated tool results and usage data. + /// + /// # Arguments + /// * `id` - Optional conversation ID. If `None`, an interactive picker is + /// shown. + async fn on_slash_rewind(&mut self, id: Option) -> anyhow::Result<()> { + let target_id = if let Some(id_str) = id { + ConversationId::parse(&id_str) + .map_err(|_| anyhow::anyhow!("Invalid conversation ID: {id_str}"))? + } else if let Some(cid) = self.state.conversation_id { + cid + } else { + // Show conversation picker + let conversations = self + .api + .get_conversations(Some(self.config.max_conversations)) + .await?; + + if conversations.is_empty() { + self.writeln_title(TitleFormat::error( + "No conversations found. Start a conversation first.", + ))?; + return Ok(()); + } + + let selected = ConversationSelector::select_conversation( + &conversations, + self.state.conversation_id, + None, + ) + .await?; + + match selected { + Some(conv) => conv.id, + None => return Ok(()), + } + }; + + // Fetch the conversation + let mut conversation = self + .api + .conversation(&target_id) + .await? + .ok_or_else(|| anyhow::anyhow!("Conversation '{target_id}' not found"))?; + + let context = conversation + .context + .as_ref() + .ok_or_else(|| anyhow::anyhow!("Conversation has no context to rewind"))?; + + if context.messages.is_empty() { + self.writeln_title(TitleFormat::error( + "Conversation has no messages to rewind.", + ))?; + return Ok(()); + } + + // Display messages with indices + self.writeln_title(TitleFormat::info(format!( + "Conversation {} — select message index to rewind to:", + target_id.into_string() + )))?; + self.writeln( + "Rewinding will discard ALL messages after the chosen index (inclusive).", + )?; + self.writeln("")?; + + let lines = context.format_messages_for_rewind(); + for line in &lines { + self.writeln(line)?; + } + + self.writeln("")?; + + // Ask user for the message index to rewind to + let default = format!("{}", context.messages.len().saturating_sub(1)); + let input = ForgeWidget::input("Rewind to message index (leave empty to cancel)") + .allow_empty(true) + .with_default(&default) + .prompt()?; + + let input = match input { + Some(s) if !s.trim().is_empty() => s.trim().to_string(), + _ => { + self.writeln_title(TitleFormat::info("Rewind cancelled."))?; + return Ok(()); + } + }; + + let keep_idx: usize = match input.parse() { + Ok(n) if n < context.messages.len() => n, + _ => { + self.writeln_title(TitleFormat::error(format!( + "Invalid index. Must be between 0 and {}.", + context.messages.len() - 1 + )))?; + return Ok(()); + } + }; + + let total_before = context.messages.len(); + + // Perform the truncation + let mut truncated_context = context.clone(); + truncated_context = truncated_context.truncate(keep_idx); + let removed = total_before - truncated_context.messages.len(); + + conversation.context = Some(truncated_context); + conversation.metadata.updated_at = Some(chrono::Utc::now()); + + self.api.upsert_conversation(conversation).await?; + + let summary = if removed > 0 { + format!("Removed {removed} messages. Now has {} messages.", keep_idx + 1) + } else { + format!("No messages removed. Still {} messages.", total_before) + }; + + self.writeln_title( + TitleFormat::info("Rewound") + .sub_title(format!("[{}] {summary}", target_id.into_string())), + )?; + + // If this is the active conversation, update the state + if self.state.conversation_id == Some(target_id) { + self.spinner.start(Some("Reloading conversation context"))?; + self.spinner.stop(None)?; + } + + Ok(()) + } + /// Renames any conversation interactively or by explicit ID and name. /// /// # Arguments From a36543c4d934074d3dfa5d5fe68e63df349baa6e Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 19:22:22 +0530 Subject: [PATCH 02/18] feat: show only user messages in rewind preview Changes rewind UX to only display user messages instead of all message types (system, assistant, tool results, images). This makes it clearer for the user to decide where to rewind to. - format_messages_for_rewind() now returns Vec<(usize, String)> with (full_index, display_string) tuples, filtered to only User role messages, numbered 1..N (1-indexed) - Added truncate_to_user_message(nth_user) which finds the Nth (0-indexed) user message and truncates everything after it - Updated handler to use 1-indexed user message selection --- crates/forge_domain/src/context.rs | 38 ++++++++++++++++++++++-------- crates/forge_main/src/ui.rs | 27 +++++++++++---------- 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index f056802cdb..afba028297 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -700,18 +700,36 @@ impl Context { self } - /// Formats messages with numbered indices for display during interactive - /// rewind. Each line shows: `[N] Role: preview_content` - pub fn format_messages_for_rewind(&self) -> Vec { + /// Truncates the conversation to keep messages up to and including the + /// Nth (0-indexed) user message. All messages after that user message's + /// assistant response are discarded. + pub fn truncate_to_user_message(mut self, nth_user: usize) -> Self { + let cut_after = self + .messages + .iter() + .enumerate() + .filter_map(|(i, entry)| entry.has_role(Role::User).then_some(i)) + .nth(nth_user); + + match cut_after { + Some(idx) if idx < self.messages.len() => { + self.messages.truncate(idx + 1); + self + } + _ => self, + } + } + + /// Formats user messages with numbered indices for display during interactive + /// rewind. Returns a list of `(full_message_index, display_string)` tuples, + /// where the display string shows a 1-indexed user message number and preview. + pub fn format_messages_for_rewind(&self) -> Vec<(usize, String)> { self.messages .iter() .enumerate() - .map(|(i, entry)| { - let role = match &**entry { - ContextMessage::Text(msg) => format!("{:?}", msg.role), - ContextMessage::Tool(_) => "Tool".to_string(), - ContextMessage::Image(_) => "Image".to_string(), - }; + .filter(|(_, entry)| entry.has_role(Role::User)) + .enumerate() + .map(|(user_idx, (full_idx, entry))| { let preview = entry.content().unwrap_or("").trim(); let max_len = 100; let preview = if preview.len() > max_len { @@ -719,7 +737,7 @@ impl Context { } else { preview.to_string() }; - format!("[{i:>3}] {role:>9}: {preview}") + (full_idx, format!("[{:>3}] User: {preview}", user_idx + 1)) }) .collect() } diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index b95b262394..37b937de1a 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2693,16 +2693,18 @@ impl A + Send + Sync> UI )?; self.writeln("")?; + // Display user messages with numbered indices let lines = context.format_messages_for_rewind(); - for line in &lines { + let user_count = lines.len(); + for (_full_idx, line) in &lines { self.writeln(line)?; } self.writeln("")?; - // Ask user for the message index to rewind to - let default = format!("{}", context.messages.len().saturating_sub(1)); - let input = ForgeWidget::input("Rewind to message index (leave empty to cancel)") + // Ask user for the user message number to rewind to + let default = format!("{user_count}"); + let input = ForgeWidget::input("Rewind to user message (1-indexed, leave empty to cancel)") .allow_empty(true) .with_default(&default) .prompt()?; @@ -2715,12 +2717,11 @@ impl A + Send + Sync> UI } }; - let keep_idx: usize = match input.parse() { - Ok(n) if n < context.messages.len() => n, + let keep_nth_user: usize = match input.parse::() { + Ok(n) if n >= 1 && n <= user_count => n - 1, // convert to 0-indexed _ => { self.writeln_title(TitleFormat::error(format!( - "Invalid index. Must be between 0 and {}.", - context.messages.len() - 1 + "Invalid selection. Must be between 1 and {user_count}.", )))?; return Ok(()); } @@ -2728,10 +2729,10 @@ impl A + Send + Sync> UI let total_before = context.messages.len(); - // Perform the truncation - let mut truncated_context = context.clone(); - truncated_context = truncated_context.truncate(keep_idx); + // Perform the truncation (keep messages up to and including the selected user message) + let truncated_context = context.clone().truncate_to_user_message(keep_nth_user); let removed = total_before - truncated_context.messages.len(); + let num_messages = truncated_context.messages.len(); conversation.context = Some(truncated_context); conversation.metadata.updated_at = Some(chrono::Utc::now()); @@ -2739,9 +2740,9 @@ impl A + Send + Sync> UI self.api.upsert_conversation(conversation).await?; let summary = if removed > 0 { - format!("Removed {removed} messages. Now has {} messages.", keep_idx + 1) + format!("Removed {removed} messages. Now has {num_messages} messages.") } else { - format!("No messages removed. Still {} messages.", total_before) + format!("No messages removed. Still {total_before} messages.") }; self.writeln_title( From d68cdbd1c5a84a4745320ea515bd5261d3299320 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 19:48:10 +0530 Subject: [PATCH 03/18] feat: revert file changes on rewind (Option A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When rewinding a conversation to a previous user message, any file changes made during the truncated portion are now automatically reverted via the snapshot system. Implementation details: - Added modified_files: Vec field to ToolResult — populated by forge_service in ToolRegistry::call() by extracting file_path from ToolCallArguments for Write/Patch/Remove tools - Added Context::modified_files_after(index) — collects all file paths that were modified by tool results after a given message index - Added undo_snapshot to the API trait + ForgeAPI impl — takes a list of file paths, calls SnapshotRepository::undo_snapshot for each, falling back to fs::remove_file if no snapshot exists (new file) - Updated on_slash_rewind to call the new API method before truncating - Updated all test fixtures with the new modified_files field Files changed: 15 files, +139 lines --- crates/forge_api/src/api.rs | 4 ++ crates/forge_api/src/forge_api.rs | 10 ++++- crates/forge_app/src/tool_registry.rs | 32 ++++++++++++-- crates/forge_domain/src/compact/summary.rs | 2 + crates/forge_domain/src/context.rs | 26 +++++++++++ crates/forge_domain/src/tools/result.rs | 6 +++ .../src/transformer/drop_reasoning_details.rs | 2 + .../src/transformer/image_handling.rs | 7 +++ crates/forge_domain/src/transformer/mod.rs | 1 + .../src/transformer/transform_tool_calls.rs | 3 ++ crates/forge_main/src/ui.rs | 44 +++++++++++++++++++ .../src/conversation/conversation_record.rs | 3 ++ .../src/conversation/conversation_repo.rs | 1 + crates/forge_repo/src/provider/anthropic.rs | 1 + crates/forge_repo/src/provider/google.rs | 1 + 15 files changed, 139 insertions(+), 4 deletions(-) diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index aafb112d49..5b2a7bd747 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -260,4 +260,8 @@ pub trait API: Sync + Send { /// Check the OAuth authentication status of an MCP server async fn mcp_auth_status(&self, server_url: &str) -> Result; + + /// Undoes the most recent snapshot for the given file path. + /// Used by the rewind command to revert file changes. + async fn undo_snapshot(&self, path: &str) -> Result<()>; } diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index b56d485bfd..b145dfc7f9 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -67,7 +67,8 @@ impl< F: CommandInfra + EnvironmentInfra + SkillRepository - + GrpcInfra, + + GrpcInfra + + SnapshotRepository, > API for ForgeAPI { async fn discover(&self) -> Result> { @@ -435,6 +436,13 @@ impl< Ok(forge_infra::mcp_auth_status(server_url, &env).await) } + async fn undo_snapshot(&self, path: &str) -> Result<()> { + self.infra + .undo_snapshot(std::path::Path::new(path)) + .await?; + Ok(()) + } + fn hydrate_channel(&self) -> Result<()> { self.infra.hydrate(); Ok(()) diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index dbfff3da06..4b43cb6968 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -5,8 +5,8 @@ use anyhow::Context; use console::style; use forge_domain::{ Agent, AgentId, AgentInput, ChatResponse, ChatResponseContent, Environment, InputModality, - Model, SystemContext, TemplateConfig, ToolCallContext, ToolCallFull, ToolCatalog, - ToolDefinition, ToolKind, ToolName, ToolOutput, ToolResult, + Model, SystemContext, TemplateConfig, ToolCallContext, ToolCallFull, + ToolCatalog, ToolDefinition, ToolKind, ToolName, ToolOutput, ToolResult, }; use forge_template::Element; use futures::future::join_all; @@ -218,9 +218,13 @@ impl> ToolReg ) -> ToolResult { let call_id = call.call_id.clone(); let tool_name = call.name.clone(); + let modified_files = Self::extract_modified_files(&call); let output = self.call_inner(agent, call, context).await; - ToolResult::new(tool_name).call_id(call_id).output(output) + ToolResult::new(tool_name) + .call_id(call_id) + .output(output) + .modified_files(modified_files) } pub async fn list(&self) -> anyhow::Result> { @@ -381,6 +385,28 @@ impl ToolRegistry { IMAGE_EXTENSIONS.iter().any(|ext| path_lower.ends_with(ext)) } + /// Extracts the file path from a tool call's arguments if it's a + /// file-modifying tool (write, patch, remove). Returns an empty vec + /// for non-modifying tools or if the path cannot be parsed. + fn extract_modified_files(call: &ToolCallFull) -> Vec { + match call.name.as_str() { + "write" | "patch" | "remove" => { + if let Ok(args) = call.arguments.parse() { + if let Some(file_path) = + args.get("file_path").and_then(|v| v.as_str()) + { + return vec![file_path.to_string()]; + } + if let Some(path) = args.get("path").and_then(|v| v.as_str()) { + return vec![path.to_string()]; + } + } + vec![] + } + _ => vec![], + } + } + /// Validates if a tool's modality requirements are supported by the current /// model. /// diff --git a/crates/forge_domain/src/compact/summary.rs b/crates/forge_domain/src/compact/summary.rs index 3416dfdba8..6683e39dcc 100644 --- a/crates/forge_domain/src/compact/summary.rs +++ b/crates/forge_domain/src/compact/summary.rs @@ -451,6 +451,7 @@ mod tests { name: ToolName::new(name), call_id: Some(ToolCallId::new(call_id)), output: ToolOutput::text("result").is_error(is_error), + modified_files: vec![], }) } @@ -805,6 +806,7 @@ mod tests { name: ToolName::new("read"), call_id: None, output: ToolOutput::text("result"), + modified_files: vec![], }), ]); diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index afba028297..172ec30de2 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -750,6 +750,19 @@ impl Context { .count() } + /// Returns the file paths modified by tool results in messages + /// after the given index (exclusive). Used by rewind to know which + /// file snapshots to revert. + pub fn modified_files_after(&self, keep_up_to: usize) -> Vec { + let mut files = Vec::new(); + for msg in &self.messages[usize::min(keep_up_to + 1, self.messages.len())..] { + if let ContextMessage::Tool(result) = &msg.message { + files.extend(result.modified_files.iter().cloned()); + } + } + files + } + /// Returns the count of assistant messages in the context pub fn assistant_message_count(&self) -> usize { self.messages @@ -963,6 +976,7 @@ mod tests { name: crate::ToolName::new("text_tool"), call_id: Some(crate::ToolCallId::new("call1")), output: crate::ToolOutput::text("Text output".to_string()), + modified_files: vec![], }, ToolResult { name: crate::ToolName::new("empty_tool"), @@ -971,6 +985,7 @@ mod tests { values: vec![crate::ToolValue::Empty], is_error: false, }, + modified_files: vec![], }, ]); @@ -989,6 +1004,7 @@ mod tests { name: crate::ToolName::new("image_tool"), call_id: Some(crate::ToolCallId::new("call1")), output: crate::ToolOutput::image(image), + modified_files: vec![], }]); let mut transformer = crate::transformer::ImageHandling::new(); @@ -1013,6 +1029,7 @@ mod tests { ], is_error: false, }, + modified_files: vec![], }]); let mut transformer = crate::transformer::ImageHandling::new(); @@ -1032,16 +1049,19 @@ mod tests { name: crate::ToolName::new("text_tool"), call_id: Some(crate::ToolCallId::new("call1")), output: crate::ToolOutput::text("Text output".to_string()), + modified_files: vec![], }, ToolResult { name: crate::ToolName::new("image_tool1"), call_id: Some(crate::ToolCallId::new("call2")), output: crate::ToolOutput::image(image1), + modified_files: vec![], }, ToolResult { name: crate::ToolName::new("image_tool2"), call_id: Some(crate::ToolCallId::new("call3")), output: crate::ToolOutput::image(image2), + modified_files: vec![], }, ]); @@ -1075,6 +1095,7 @@ mod tests { ], is_error: false, }, + modified_files: vec![], }]); let mut transformer = crate::transformer::ImageHandling::new(); @@ -1093,6 +1114,7 @@ mod tests { values: vec![crate::ToolValue::Image(image)], is_error: true, }, + modified_files: vec![], }]); let mut transformer = crate::transformer::ImageHandling::new(); @@ -1438,11 +1460,13 @@ mod tests { name: crate::ToolName::new("tool1"), call_id: Some(crate::ToolCallId::new("call1")), output: crate::ToolOutput::text("Result 1".to_string()), + modified_files: vec![], }, ToolResult { name: crate::ToolName::new("tool2"), call_id: Some(crate::ToolCallId::new("call2")), output: crate::ToolOutput::text("Result 2".to_string()), + modified_files: vec![], }, ]); @@ -1595,6 +1619,7 @@ mod tests { name: crate::ToolName::new("fs_search"), call_id: Some(crate::ToolCallId::new("call1")), output: crate::ToolOutput::text("Search results: Found 3 items".to_string()), + modified_files: vec![], }); let actual = fixture.token_count_approx(); let expected = 8; // 30 chars / 4 = 8 tokens (rounded up) @@ -1609,6 +1634,7 @@ mod tests { name: crate::ToolName::new("screenshot"), call_id: Some(crate::ToolCallId::new("call1")), output: crate::ToolOutput::image(fixture_image), + modified_files: vec![], }); let actual = fixture.token_count_approx(); let expected = 0; // Images are not counted in token approximation diff --git a/crates/forge_domain/src/tools/result.rs b/crates/forge_domain/src/tools/result.rs index 1f68ca294f..10518fe1c3 100644 --- a/crates/forge_domain/src/tools/result.rs +++ b/crates/forge_domain/src/tools/result.rs @@ -14,6 +14,10 @@ pub struct ToolResult { pub call_id: Option, #[setters(skip)] pub output: ToolOutput, + /// File paths modified by this tool call (for undo/rewind purposes). + /// Populated by file-modifying tools (Write, Patch, Remove). + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub modified_files: Vec, } impl ToolResult { @@ -22,6 +26,7 @@ impl ToolResult { name: name.into(), call_id: Default::default(), output: Default::default(), + modified_files: vec![], } } @@ -75,6 +80,7 @@ impl From for ToolResult { name: value.name, call_id: value.call_id, output: Default::default(), + modified_files: vec![], } } } diff --git a/crates/forge_domain/src/transformer/drop_reasoning_details.rs b/crates/forge_domain/src/transformer/drop_reasoning_details.rs index e6a016feb7..1a2c0ac1c7 100644 --- a/crates/forge_domain/src/transformer/drop_reasoning_details.rs +++ b/crates/forge_domain/src/transformer/drop_reasoning_details.rs @@ -85,6 +85,7 @@ mod tests { name: ToolName::new("test_tool"), call_id: Some(ToolCallId::new("call_123")), output: ToolOutput::text("Tool result".to_string()), + modified_files: vec![], }]) } @@ -172,6 +173,7 @@ mod tests { name: ToolName::new("preserve_tool"), call_id: Some(ToolCallId::new("call_preserve")), output: ToolOutput::text("Tool output".to_string()), + modified_files: vec![], }]); let mut transformer = DropReasoningDetails; diff --git a/crates/forge_domain/src/transformer/image_handling.rs b/crates/forge_domain/src/transformer/image_handling.rs index c301b3778a..bc7cc741f7 100644 --- a/crates/forge_domain/src/transformer/image_handling.rs +++ b/crates/forge_domain/src/transformer/image_handling.rs @@ -100,6 +100,7 @@ mod tests { ], is_error: false, }, + modified_files: vec![], }]) } @@ -114,11 +115,13 @@ mod tests { name: ToolName::new("image_tool_1"), call_id: Some(ToolCallId::new("call_1")), output: ToolOutput::image(image1), + modified_files: vec![], }, ToolResult { name: ToolName::new("image_tool_2"), call_id: Some(ToolCallId::new("call_2")), output: ToolOutput::image(image2), + modified_files: vec![], }, ]) } @@ -141,6 +144,7 @@ mod tests { name: ToolName::new("text_tool"), call_id: Some(ToolCallId::new("call_text")), output: ToolOutput::text("Just text output".to_string()), + modified_files: vec![], }]); let mut transformer = ImageHandling::new(); @@ -178,6 +182,7 @@ mod tests { ], is_error: false, }, + modified_files: vec![], }]); let mut transformer = ImageHandling::new(); @@ -201,6 +206,7 @@ mod tests { ], is_error: true, }, + modified_files: vec![], }]); let mut transformer = ImageHandling::new(); @@ -237,6 +243,7 @@ mod tests { name: ToolName::new("image_tool"), call_id: Some(ToolCallId::new("call_preserve")), output: ToolOutput::image(image), + modified_files: vec![], }]); let mut transformer = ImageHandling::new(); diff --git a/crates/forge_domain/src/transformer/mod.rs b/crates/forge_domain/src/transformer/mod.rs index 1f4ccc91b7..5168d8089f 100644 --- a/crates/forge_domain/src/transformer/mod.rs +++ b/crates/forge_domain/src/transformer/mod.rs @@ -133,6 +133,7 @@ mod tests { name: ToolName::new("test_tool"), call_id: Some(ToolCallId::new("call_123")), output: ToolOutput::text("Tool result text".to_string()), + modified_files: vec![], }]) } diff --git a/crates/forge_domain/src/transformer/transform_tool_calls.rs b/crates/forge_domain/src/transformer/transform_tool_calls.rs index da063a8886..0b98650c53 100644 --- a/crates/forge_domain/src/transformer/transform_tool_calls.rs +++ b/crates/forge_domain/src/transformer/transform_tool_calls.rs @@ -119,6 +119,7 @@ mod tests { name: ToolName::new("test_tool"), call_id: Some(ToolCallId::new("call_123")), output: ToolOutput::text("Tool result text".to_string()), + modified_files: vec![], }]) } @@ -137,6 +138,7 @@ mod tests { ], is_error: false, }, + modified_files: vec![], }]) } @@ -206,6 +208,7 @@ mod tests { name: ToolName::new("empty_tool"), call_id: Some(ToolCallId::new("call_empty")), output: ToolOutput { values: vec![ToolValue::Empty], is_error: false }, + modified_files: vec![], }]); let mut transformer = TransformToolCalls::new(); diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 37b937de1a..8523b64305 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2729,6 +2729,22 @@ impl A + Send + Sync> UI let total_before = context.messages.len(); + // Find the full index of the Nth user message before truncating + // so we can collect the files that will be removed. + let cut_index = context + .messages + .iter() + .enumerate() + .filter_map(|(i, entry)| entry.has_role(Role::User).then_some(i)) + .nth(keep_nth_user); + + // Collect file paths that were modified by tool results in messages + // that will be removed. These files' snapshots need to be reverted. + let modified_files: Vec = match cut_index { + Some(idx) => context.modified_files_after(idx), + None => vec![], + }; + // Perform the truncation (keep messages up to and including the selected user message) let truncated_context = context.clone().truncate_to_user_message(keep_nth_user); let removed = total_before - truncated_context.messages.len(); @@ -2739,6 +2755,34 @@ impl A + Send + Sync> UI self.api.upsert_conversation(conversation).await?; + // Revert file modifications from the removed messages (best-effort) + if !modified_files.is_empty() { + self.writeln_title(TitleFormat::info("Reverting file changes..."))?; + let mut unique_files = std::collections::BTreeSet::new(); + for f in &modified_files { + unique_files.insert(f.clone()); + } + let mut failed = 0u32; + for file_path in &unique_files { + match self.api.undo_snapshot(file_path).await { + Ok(_) => { + tracing::info!(file = %file_path, "Rewind reverted snapshot"); + } + Err(e) => { + tracing::warn!(file = %file_path, error = %e, "Failed to revert snapshot during rewind"); + failed += 1; + } + } + } + let file_count = unique_files.len(); + let status = if failed == 0 { + format!("Reverted {file_count} file(s).") + } else { + format!("Reverted {} of {file_count} file(s). {failed} failed.", file_count - failed as usize) + }; + self.writeln(status)?; + } + let summary = if removed > 0 { format!("Removed {removed} messages. Now has {num_messages} messages.") } else { diff --git a/crates/forge_repo/src/conversation/conversation_record.rs b/crates/forge_repo/src/conversation/conversation_record.rs index 7df99bf5a3..8be3654c8c 100644 --- a/crates/forge_repo/src/conversation/conversation_record.rs +++ b/crates/forge_repo/src/conversation/conversation_record.rs @@ -467,6 +467,7 @@ pub(super) struct ToolResultRecord { name: ToolNameRecord, call_id: Option, output: ToolOutputRecord, + modified_files: Vec, } impl From<&forge_domain::ToolResult> for ToolResultRecord { @@ -475,6 +476,7 @@ impl From<&forge_domain::ToolResult> for ToolResultRecord { name: ToolNameRecord::from(&result.name), call_id: result.call_id.as_ref().map(ToolCallIdRecord::from), output: ToolOutputRecord::from(&result.output), + modified_files: result.modified_files.clone(), } } } @@ -487,6 +489,7 @@ impl TryFrom for forge_domain::ToolResult { name: record.name.into(), call_id: record.call_id.map(Into::into), output: record.output.try_into()?, + modified_files: record.modified_files, }) } } diff --git a/crates/forge_repo/src/conversation/conversation_repo.rs b/crates/forge_repo/src/conversation/conversation_repo.rs index eeef25af71..3852611ebb 100644 --- a/crates/forge_repo/src/conversation/conversation_repo.rs +++ b/crates/forge_repo/src/conversation/conversation_repo.rs @@ -716,6 +716,7 @@ mod tests { is_error: false, values: vec![ToolValue::Text("Result text".to_string()), ToolValue::Empty], }, + modified_files: vec![], }) .into(), forge_domain::MessageEntry { diff --git a/crates/forge_repo/src/provider/anthropic.rs b/crates/forge_repo/src/provider/anthropic.rs index c9449d1bca..ae293519be 100644 --- a/crates/forge_repo/src/provider/anthropic.rs +++ b/crates/forge_repo/src/provider/anthropic.rs @@ -576,6 +576,7 @@ mod tests { name: ToolName::new("math"), call_id: Some(ToolCallId::new("math-1")), output: ToolOutput::text(serde_json::json!({"result": 4}).to_string()), + modified_files: vec![], }]) .tool_choice(ToolChoice::Call(ToolName::new("math"))); let request = Request::try_from(context) diff --git a/crates/forge_repo/src/provider/google.rs b/crates/forge_repo/src/provider/google.rs index c70b2b28c8..a4dbe77415 100644 --- a/crates/forge_repo/src/provider/google.rs +++ b/crates/forge_repo/src/provider/google.rs @@ -396,6 +396,7 @@ mod tests { name: ToolName::new("math"), call_id: Some(ToolCallId::new("math-1")), output: ToolOutput::text(serde_json::json!({"result": 4}).to_string()), + modified_files: vec![], }]) .tool_choice(ToolChoice::Call(ToolName::new("math"))); From 73e68f65da72e44de10e6c2c9dd95b10cf3f7f2f Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 20:03:45 +0530 Subject: [PATCH 04/18] feat: replace input prompt with TUI message selector for :rewind Replaces the text-based numeric input prompt with a full TUI selector (arrow keys + Enter) for choosing the rewind target message. Same interactive pattern as the conversation picker, provider/model selectors. - Displays user messages as selectable rows in the nucleo-backed fuzzy search TUI (ForgeWidget::select_rows) - Cursor starts on the last message by default (initial_raw) so the common no-op case is just Enter - Up/down arrow keys, PageUp/Down, fuzzy search filtering all work - Enter selects, Esc/Ctrl+C cancels - Removed 32 lines of text-display + input-prompt boilerplate --- crates/forge_main/src/ui.rs | 66 +++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 8523b64305..6872d3fc07 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2683,49 +2683,51 @@ impl A + Send + Sync> UI return Ok(()); } - // Display messages with indices - self.writeln_title(TitleFormat::info(format!( - "Conversation {} — select message index to rewind to:", - target_id.into_string() - )))?; - self.writeln( - "Rewinding will discard ALL messages after the chosen index (inclusive).", - )?; - self.writeln("")?; - - // Display user messages with numbered indices + // Build and show the interactive TUI message selector let lines = context.format_messages_for_rewind(); let user_count = lines.len(); - for (_full_idx, line) in &lines { - self.writeln(line)?; + if user_count == 0 { + self.writeln_title(TitleFormat::error( + "No user messages to rewind to.", + ))?; + return Ok(()); } - self.writeln("")?; + let last_full_idx = lines.last().map(|(fi, _)| fi.to_string()).unwrap_or_default(); + let mut rows: Vec = Vec::with_capacity(lines.len() + 1); + rows.push(SelectRow::header(format!("{:>3} Message", "#"))); + for (full_idx, display) in &lines { + rows.push( + SelectRow::new(full_idx.to_string(), display.clone()) + .search(display.clone()), + ); + } - // Ask user for the user message number to rewind to - let default = format!("{user_count}"); - let input = ForgeWidget::input("Rewind to user message (1-indexed, leave empty to cancel)") - .allow_empty(true) - .with_default(&default) - .prompt()?; + let selected = + tokio::task::spawn_blocking(move || -> anyhow::Result> { + Ok(ForgeWidget::select_rows("Rewind to message", rows) + .header_lines(1_usize) + .initial_raw(last_full_idx) + .prompt()?) + }) + .await??; - let input = match input { - Some(s) if !s.trim().is_empty() => s.trim().to_string(), - _ => { + let full_idx = match selected { + Some(row) => row + .raw + .parse::() + .map_err(|_| anyhow::anyhow!("Invalid message index"))?, + None => { self.writeln_title(TitleFormat::info("Rewind cancelled."))?; return Ok(()); } }; - let keep_nth_user: usize = match input.parse::() { - Ok(n) if n >= 1 && n <= user_count => n - 1, // convert to 0-indexed - _ => { - self.writeln_title(TitleFormat::error(format!( - "Invalid selection. Must be between 1 and {user_count}.", - )))?; - return Ok(()); - } - }; + // Find the 0-indexed user message position for the existing truncation method + let keep_nth_user = lines + .iter() + .position(|(fi, _)| *fi == full_idx) + .ok_or_else(|| anyhow::anyhow!("Selected message index {full_idx} not found"))?; let total_before = context.messages.len(); From a255ad8345f5f0d6dbb6c8dbf1b2e1016a7fec21 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 20:09:39 +0530 Subject: [PATCH 05/18] refactor: show only message content in rewind selector, drop header row The user message list in :rewind now shows just the plain text content (e.g. 'write a python script...') instead of '[1] User: write a python script...'. The header row (# Message) is removed since there's only one data column. --- crates/forge_domain/src/context.rs | 2 +- crates/forge_main/src/ui.rs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index 172ec30de2..f9fe0ec4a3 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -737,7 +737,7 @@ impl Context { } else { preview.to_string() }; - (full_idx, format!("[{:>3}] User: {preview}", user_idx + 1)) + (full_idx, preview) }) .collect() } diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 6872d3fc07..ffcfb5d0a2 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2694,8 +2694,7 @@ impl A + Send + Sync> UI } let last_full_idx = lines.last().map(|(fi, _)| fi.to_string()).unwrap_or_default(); - let mut rows: Vec = Vec::with_capacity(lines.len() + 1); - rows.push(SelectRow::header(format!("{:>3} Message", "#"))); + let mut rows: Vec = Vec::with_capacity(lines.len()); for (full_idx, display) in &lines { rows.push( SelectRow::new(full_idx.to_string(), display.clone()) @@ -2706,7 +2705,6 @@ impl A + Send + Sync> UI let selected = tokio::task::spawn_blocking(move || -> anyhow::Result> { Ok(ForgeWidget::select_rows("Rewind to message", rows) - .header_lines(1_usize) .initial_raw(last_full_idx) .prompt()?) }) From 49c889f07fb61c1e3efca345707e7fb59935a901 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 20:12:45 +0530 Subject: [PATCH 06/18] chore: fix unused variable warning in format_messages_for_rewind --- crates/forge_domain/src/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index f9fe0ec4a3..560b6af9fd 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -729,7 +729,7 @@ impl Context { .enumerate() .filter(|(_, entry)| entry.has_role(Role::User)) .enumerate() - .map(|(user_idx, (full_idx, entry))| { + .map(|(_user_idx, (full_idx, entry))| { let preview = entry.content().unwrap_or("").trim(); let max_len = 100; let preview = if preview.len() > max_len { From fd1dc62c22d3f74032220224622d4037acc55271 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 20:27:50 +0530 Subject: [PATCH 07/18] feat: strip XML tags from message previews in rewind selector User messages are often wrapped in ... or ... tags from the event context template. Strip these before showing in the rewind TUI selector so the previews show clean text. - Added strip_xml_tags() to forge_domain::xml which removes ... pairs for known event tags - Applied in Context::format_messages_for_rewind() --- crates/forge_domain/src/context.rs | 2 ++ crates/forge_domain/src/xml.rs | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index 560b6af9fd..cd52cf1258 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -17,6 +17,7 @@ fn is_false(value: &bool) -> bool { use crate::temperature::Temperature; use crate::top_k::TopK; use crate::top_p::TopP; +use crate::xml::strip_xml_tags; use crate::{ Attachment, AttachmentContent, ConversationId, EventValue, Image, MessagePhase, ModelId, ReasoningFull, ToolChoice, ToolDefinition, ToolOutput, ToolValue, Usage, @@ -731,6 +732,7 @@ impl Context { .enumerate() .map(|(_user_idx, (full_idx, entry))| { let preview = entry.content().unwrap_or("").trim(); + let preview = strip_xml_tags(preview); let max_len = 100; let preview = if preview.len() > max_len { format!("{}...", &preview[..max_len]) diff --git a/crates/forge_domain/src/xml.rs b/crates/forge_domain/src/xml.rs index 7ba5fbadfc..09c416d785 100644 --- a/crates/forge_domain/src/xml.rs +++ b/crates/forge_domain/src/xml.rs @@ -58,6 +58,16 @@ pub fn remove_tag_with_prefix(text: &str, prefix: &str) -> String { result } +/// Removes all XML/HTML tags from the text, keeping only the content between tags. +/// Multiple whitespace characters are collapsed into a single space. +pub fn strip_xml_tags(text: &str) -> String { + let tag_pattern = regex::Regex::new(r"<[^>]*>").unwrap(); + let result = tag_pattern.replace_all(text, "").to_string(); + // Collapse multiple whitespace characters into a single space + let re_whitespace = regex::Regex::new(r"\s+").unwrap(); + re_whitespace.replace_all(&result, " ").trim().to_string() +} + #[cfg(test)] mod tests { use pretty_assertions::assert_eq; From b8d9bd59b08ba3c205015cad6e38b6530b8665b4 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 20:37:24 +0530 Subject: [PATCH 08/18] fix: exclude auto-generated user messages from rewind selector The rewind selector was showing auto-injected messages (externally modified files notification, piped input, resume todos) alongside actual user-typed messages because they all use Role::User. Fix: use the 'droppable' field as the discriminator. User-typed messages have droppable=false while system-generated user messages have droppable=true. Also exclude non-Text variants (Tool, Image). - format_messages_for_rewind: filter by Text+User+!droppable - user_message_count: same filter for consistency --- crates/forge_domain/src/context.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index cd52cf1258..a45f7af5f4 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -728,7 +728,12 @@ impl Context { self.messages .iter() .enumerate() - .filter(|(_, entry)| entry.has_role(Role::User)) + .filter(|(_, entry)| { + match &entry.message { + ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, + _ => false, + } + }) .enumerate() .map(|(_user_idx, (full_idx, entry))| { let preview = entry.content().unwrap_or("").trim(); @@ -748,7 +753,12 @@ impl Context { pub fn user_message_count(&self) -> usize { self.messages .iter() - .filter(|msg| msg.has_role(Role::User)) + .filter(|msg| { + match &msg.message { + ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, + _ => false, + } + }) .count() } From eefd9b57473f1f12fde358ca25c4fb7e1833c34f Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Fri, 15 May 2026 20:50:34 +0530 Subject: [PATCH 09/18] fix: two bugs causing rewind file revert to fail Bug #1 - truncate_to_user_message used has_role(Role::User) which counts auto-generated droppable messages (externally modified files notification, piped input, resume todos). This caused truncation to the wrong index, losing user-typed messages and skipping file reversion. Bug #2 - cut_index in on_slash_rewind (ui.rs) used has_role(Role::User) for the same reason, making modified_files_after look at the wrong slice of messages. Files that should have been reverted were missed. Both now use the same filter as format_messages_for_rewind(): ContextMessage::Text(msg) with msg.role == Role::User && !msg.droppable --- crates/forge_domain/src/context.rs | 9 ++++++++- crates/forge_main/src/ui.rs | 9 ++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index a45f7af5f4..cf457d485e 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -709,7 +709,14 @@ impl Context { .messages .iter() .enumerate() - .filter_map(|(i, entry)| entry.has_role(Role::User).then_some(i)) + .filter_map(|(i, entry)| { + match &entry.message { + ContextMessage::Text(msg) if msg.role == Role::User && !msg.droppable => { + Some(i) + } + _ => None, + } + }) .nth(nth_user); match cut_after { diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index ffcfb5d0a2..18147fe275 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2735,7 +2735,14 @@ impl A + Send + Sync> UI .messages .iter() .enumerate() - .filter_map(|(i, entry)| entry.has_role(Role::User).then_some(i)) + .filter_map(|(i, entry)| { + match &entry.message { + ContextMessage::Text(msg) if msg.role == Role::User && !msg.droppable => { + Some(i) + } + _ => None, + } + }) .nth(keep_nth_user); // Collect file paths that were modified by tool results in messages From fcc9a34bf10d1755681912b01ad267a426be3c5f Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 00:01:14 +0530 Subject: [PATCH 10/18] fix(rewind): track multi_patch as modifying tool and preserve operation order in rewind - Add to the list of modifying tools in - Remove BTreeSet deduplication in rewind to preserve operation order - Iterate modifications in reverse order for correct undo sequence - Use 'file change(s)' terminology to reflect per-operation tracking Co-Authored-By: ForgeCode --- crates/forge_app/src/tool_registry.rs | 2 +- crates/forge_main/src/ui.rs | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index 4b43cb6968..c1ffc7bbe3 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -390,7 +390,7 @@ impl ToolRegistry { /// for non-modifying tools or if the path cannot be parsed. fn extract_modified_files(call: &ToolCallFull) -> Vec { match call.name.as_str() { - "write" | "patch" | "remove" => { + "write" | "patch" | "multi_patch" | "remove" => { if let Ok(args) = call.arguments.parse() { if let Some(file_path) = args.get("file_path").and_then(|v| v.as_str()) diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 18147fe275..bff24cadf9 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2765,12 +2765,9 @@ impl A + Send + Sync> UI // Revert file modifications from the removed messages (best-effort) if !modified_files.is_empty() { self.writeln_title(TitleFormat::info("Reverting file changes..."))?; - let mut unique_files = std::collections::BTreeSet::new(); - for f in &modified_files { - unique_files.insert(f.clone()); - } let mut failed = 0u32; - for file_path in &unique_files { + let total_undos = modified_files.len(); + for file_path in modified_files.iter().rev() { match self.api.undo_snapshot(file_path).await { Ok(_) => { tracing::info!(file = %file_path, "Rewind reverted snapshot"); @@ -2781,11 +2778,10 @@ impl A + Send + Sync> UI } } } - let file_count = unique_files.len(); let status = if failed == 0 { - format!("Reverted {file_count} file(s).") + format!("Reverted {total_undos} file change(s).") } else { - format!("Reverted {} of {file_count} file(s). {failed} failed.", file_count - failed as usize) + format!("Reverted {} of {total_undos} file change(s). {failed} failed.", total_undos - failed as usize) }; self.writeln(status)?; } From 50918eb24c2b6f6c3fe87786e88bc4b29bd2ac80 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 01:31:19 +0530 Subject: [PATCH 11/18] feat(rewind): comprehensive rewind enhancements - Truncation semantics: Changed truncate_to_user_message to exclude the target user message (was inclusive), giving cleaner rewinds. - File creation undo: Snapshot non-existent files with a .none marker so undo can delete files that were created after the rewind point. - Path detection: Extract modified file paths from tool output XML tags for accurate absolute paths; fall back to argument-based extraction. - Message preview cleanup: Added clean_user_prompt to extract feedback tag content or strip meta tags for cleaner display. - Shell integration: Added :rewind command to the zsh plugin, writing the rewound message into the user's buffer for editing/resubmission. - Snapshot coordination: Always snapshot before fs_write (not just when file exists) and before plan_create to support full undo coverage. - CLI command: Added conversation rewind subcommand with optional conversation ID. Co-Authored-By: ForgeCode --- crates/forge_app/src/tool_registry.rs | 41 ++++++++- crates/forge_domain/src/context.rs | 84 +++++++++++++++---- crates/forge_domain/src/xml.rs | 60 ++++++++++++- crates/forge_main/src/cli.rs | 6 ++ crates/forge_main/src/model.rs | 1 + crates/forge_main/src/ui.rs | 47 ++++++----- .../src/tool_services/fs_write.rs | 6 +- .../src/tool_services/plan_create.rs | 10 ++- crates/forge_snaps/src/service.rs | 50 +++++++++-- shell-plugin/lib/actions/conversation.zsh | 37 ++++++++ shell-plugin/lib/dispatcher.zsh | 3 + 11 files changed, 292 insertions(+), 53 deletions(-) diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index c1ffc7bbe3..c634dcfbff 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -218,8 +218,45 @@ impl> ToolReg ) -> ToolResult { let call_id = call.call_id.clone(); let tool_name = call.name.clone(); - let modified_files = Self::extract_modified_files(&call); - let output = self.call_inner(agent, call, context).await; + let output = self.call_inner(agent, call.clone(), context).await; + + let mut modified_files = Vec::new(); + match &output { + Ok(output) => { + if let Some(text) = output.as_str() { + // Extract path from plan_created or file_created tags + // These tags usually contain absolute paths + if let Some(tag) = forge_domain::extract_tag(text, "plan_created") { + if let Some(path) = forge_domain::extract_attribute(tag, "path") { + modified_files.push(path); + } + } else if let Some(tag) = forge_domain::extract_tag(text, "file_created") { + if let Some(path) = forge_domain::extract_attribute(tag, "path") { + modified_files.push(path); + } + } else if let Some(tag) = forge_domain::extract_tag(text, "file_overwritten") { + if let Some(path) = forge_domain::extract_attribute(tag, "path") { + modified_files.push(path); + } + } else if let Some(tag) = forge_domain::extract_tag(text, "file_diff") { + if let Some(path) = forge_domain::extract_attribute(tag, "path") { + modified_files.push(path); + } + } else if let Some(tag) = forge_domain::extract_tag(text, "file_removed") { + if let Some(path) = forge_domain::extract_attribute(tag, "path") { + modified_files.push(path); + } + } + } + } + _ => {} + } + + // Fallback to extraction from arguments if output didn't yield anything + // This is important for relative paths provided in tool arguments + if modified_files.is_empty() { + modified_files = Self::extract_modified_files(&call); + } ToolResult::new(tool_name) .call_id(call_id) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index cf457d485e..8d2dfd8d06 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -17,7 +17,7 @@ fn is_false(value: &bool) -> bool { use crate::temperature::Temperature; use crate::top_k::TopK; use crate::top_p::TopP; -use crate::xml::strip_xml_tags; +use crate::xml::{clean_user_prompt, strip_xml_tags}; use crate::{ Attachment, AttachmentContent, ConversationId, EventValue, Image, MessagePhase, ModelId, ReasoningFull, ToolChoice, ToolDefinition, ToolOutput, ToolValue, Usage, @@ -701,11 +701,10 @@ impl Context { self } - /// Truncates the conversation to keep messages up to and including the - /// Nth (0-indexed) user message. All messages after that user message's - /// assistant response are discarded. + /// Truncates the conversation to keep messages up to (but excluding) the + /// Nth (0-indexed) user message. All subsequent messages are discarded. pub fn truncate_to_user_message(mut self, nth_user: usize) -> Self { - let cut_after = self + let cut_at = self .messages .iter() .enumerate() @@ -719,9 +718,9 @@ impl Context { }) .nth(nth_user); - match cut_after { + match cut_at { Some(idx) if idx < self.messages.len() => { - self.messages.truncate(idx + 1); + self.messages.truncate(idx); self } _ => self, @@ -743,8 +742,9 @@ impl Context { }) .enumerate() .map(|(_user_idx, (full_idx, entry))| { - let preview = entry.content().unwrap_or("").trim(); - let preview = strip_xml_tags(preview); + let content = entry.content().unwrap_or("").trim(); + let cleaned = clean_user_prompt(content); + let preview = strip_xml_tags(&cleaned); let max_len = 100; let preview = if preview.len() > max_len { format!("{}...", &preview[..max_len]) @@ -770,18 +770,27 @@ impl Context { } /// Returns the file paths modified by tool results in messages - /// after the given index (exclusive). Used by rewind to know which + /// at or after the given index (inclusive). Used by rewind to know which /// file snapshots to revert. - pub fn modified_files_after(&self, keep_up_to: usize) -> Vec { + pub fn modified_files_from(&self, from_index: usize) -> Vec { let mut files = Vec::new(); - for msg in &self.messages[usize::min(keep_up_to + 1, self.messages.len())..] { - if let ContextMessage::Tool(result) = &msg.message { - files.extend(result.modified_files.iter().cloned()); + if from_index < self.messages.len() { + for msg in &self.messages[from_index..] { + if let ContextMessage::Tool(result) = &msg.message { + files.extend(result.modified_files.iter().cloned()); + } } } files } + /// Returns the file paths modified by tool results in messages + /// after the given index (exclusive). Used by rewind to know which + /// file snapshots to revert. + pub fn modified_files_after(&self, keep_up_to: usize) -> Vec { + self.modified_files_from(keep_up_to + 1) + } + /// Returns the count of assistant messages in the context pub fn assistant_message_count(&self) -> usize { self.messages @@ -1797,6 +1806,53 @@ mod tests { assert_eq!(actual, expected); } + #[test] + fn test_truncate_to_user_message_exclusive() { + let context = Context::default() + .add_message(ContextMessage::user("U1", None)) // idx 0 + .add_message(TextMessage::assistant("A1", None, None)) // idx 1 + .add_message(ContextMessage::user("U2", None)) // idx 2 + .add_message(TextMessage::assistant("A2", None, None)); // idx 3 + + // Rewind to U2 (nth_user = 1) -> keeps [U1, A1] + let rewound = context.clone().truncate_to_user_message(1); + assert_eq!(rewound.messages.len(), 2); + assert_eq!(rewound.messages[0].content().unwrap(), "U1"); + assert_eq!(rewound.messages[1].content().unwrap(), "A1"); + + // Rewind to U1 (nth_user = 0) -> keeps [] + let rewound = context.truncate_to_user_message(0); + assert_eq!(rewound.messages.len(), 0); + } + + #[test] + fn test_modified_files_from() { + use crate::{ToolName, ToolOutput, ToolResult}; + let context = Context::default() + .add_message(ContextMessage::user("U1", None)) + .add_message(ContextMessage::Tool(ToolResult { + name: ToolName::new("write"), + call_id: None, + output: ToolOutput::text("ok"), + modified_files: vec!["file1.txt".to_string()], + })) + .add_message(ContextMessage::user("U2", None)) + .add_message(ContextMessage::Tool(ToolResult { + name: ToolName::new("patch"), + call_id: None, + output: ToolOutput::text("ok"), + modified_files: vec!["file2.txt".to_string()], + })); + + // From U2 (idx 2) + let files = context.modified_files_from(2); + assert_eq!(files, vec!["file2.txt".to_string()]); + + // From U1 (idx 0) + let files = context.modified_files_from(0); + assert_eq!(files, vec!["file1.txt".to_string(), "file2.txt".to_string()]); + } + /// Regression test: when both `reasoning` (raw text) and /// `reasoning_details` (structured, with a cryptographic signature) are /// present, `append_message` must NOT create a duplicate thinking block diff --git a/crates/forge_domain/src/xml.rs b/crates/forge_domain/src/xml.rs index 09c416d785..ec483797d9 100644 --- a/crates/forge_domain/src/xml.rs +++ b/crates/forge_domain/src/xml.rs @@ -1,3 +1,14 @@ +/// Extracts a full XML tag (including brackets) by its name +pub fn extract_tag<'a>(text: &'a str, tag_name: &str) -> Option<&'a str> { + let opening_pattern = format!(r"<{tag_name}(?:\s[^>]*?)?>"); + if let Ok(regex) = regex::Regex::new(&opening_pattern) { + if let Some(mat) = regex.find(text) { + return Some(mat.as_str()); + } + } + None +} + /// Extracts content between the specified XML-style tags /// /// # Arguments @@ -58,6 +69,37 @@ pub fn remove_tag_with_prefix(text: &str, prefix: &str) -> String { result } +/// Cleans a user prompt by extracting content from tags if present, +/// or stripping all XML tags and meta-information. +pub fn clean_user_prompt(text: &str) -> String { + // 1. Try to extract content from tag + if let Some(content) = extract_tag_content(text, "feedback") { + return content.to_string(); + } + + // 2. Remove known meta tags with their content + let mut cleaned = remove_tag_with_prefix(text, "system_"); + cleaned = remove_tag_with_prefix(&cleaned, "context_"); + + // 3. Strip all remaining tags but preserve newlines (unlike strip_xml_tags) + let tag_pattern = regex::Regex::new(r"<[^>]*>").unwrap(); + let result = tag_pattern.replace_all(&cleaned, "").to_string(); + + // Trim while preserving internal structure + result.trim().to_string() +} + +/// Extracts the value of an attribute from an XML tag +pub fn extract_attribute(tag: &str, attr_name: &str) -> Option { + let pattern = format!(r#"{attr_name}="([^"]*)""#, attr_name = attr_name); + if let Ok(regex) = regex::Regex::new(&pattern) { + if let Some(captures) = regex.captures(tag) { + return captures.get(1).map(|m| m.as_str().to_string()); + } + } + None +} + /// Removes all XML/HTML tags from the text, keeping only the content between tags. /// Multiple whitespace characters are collapsed into a single space. pub fn strip_xml_tags(text: &str) -> String { @@ -169,10 +211,20 @@ mod tests { } #[test] - fn test_with_duplicate_closing_tags() { - let fixture = "123"; - let actual = extract_tag_content(fixture, "foo").unwrap(); - let expected = "123"; + fn test_clean_user_prompt_with_tags() { + let fixture = "add feature to determine receipe from images using vision llm models\n::: 2026-05-16\n::: "; + let actual = clean_user_prompt(fixture); + // Should extract ONLY feedback and trim + let expected = "add feature to determine receipe from images using vision llm models"; + assert_eq!(actual, expected); + } + + #[test] + fn test_clean_user_prompt_without_feedback() { + let fixture = "Just plain text 2026"; + let actual = clean_user_prompt(fixture); + // Should strip system_date and its tags + let expected = "Just plain text"; assert_eq!(actual, expected); } } diff --git a/crates/forge_main/src/cli.rs b/crates/forge_main/src/cli.rs index a4c859bcd7..0a031c4164 100644 --- a/crates/forge_main/src/cli.rs +++ b/crates/forge_main/src/cli.rs @@ -747,6 +747,12 @@ pub enum ConversationCommand { id: ConversationId, }, + /// Rewind a conversation to an earlier message. + Rewind { + /// Conversation ID to rewind. + id: Option, + }, + /// Show last assistant message. Show { /// Conversation ID. diff --git a/crates/forge_main/src/model.rs b/crates/forge_main/src/model.rs index 94913b7d29..0ac79be6ac 100644 --- a/crates/forge_main/src/model.rs +++ b/crates/forge_main/src/model.rs @@ -143,6 +143,7 @@ impl ForgeCommandManager { | "suggest" | "s" | "clone" + | "rewind" | "conversation-rename" | "copy" | "workspace-sync" diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index bff24cadf9..7c3076ca82 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -19,6 +19,7 @@ use forge_config::ForgeConfig; use forge_display::MarkdownFormat; use forge_domain::{ AuthMethod, ChatResponseContent, ConsoleWriter, ContextMessage, Role, TitleFormat, UserCommand, + clean_user_prompt, }; use forge_fs::ForgeFS; use forge_select::{ForgeWidget, SelectRow}; @@ -917,6 +918,9 @@ impl A + Send + Sync> UI self.writeln_title(TitleFormat::info(format!("Resumed conversation: {id}")))?; // Interactive mode will be handled by the main loop } + ConversationCommand::Rewind { id } => { + self.on_slash_rewind(id.map(|i| i.to_string())).await?; + } ConversationCommand::Show { id, md } => { let conversation = self.validate_conversation_exists(&id).await?; @@ -2729,30 +2733,17 @@ impl A + Send + Sync> UI let total_before = context.messages.len(); - // Find the full index of the Nth user message before truncating - // so we can collect the files that will be removed. - let cut_index = context - .messages - .iter() - .enumerate() - .filter_map(|(i, entry)| { - match &entry.message { - ContextMessage::Text(msg) if msg.role == Role::User && !msg.droppable => { - Some(i) - } - _ => None, - } - }) - .nth(keep_nth_user); - // Collect file paths that were modified by tool results in messages // that will be removed. These files' snapshots need to be reverted. - let modified_files: Vec = match cut_index { - Some(idx) => context.modified_files_after(idx), - None => vec![], - }; + // We use full_idx because it's the index of the user message we are rewinding AT. + let modified_files = context.modified_files_from(full_idx); + + // Perform the truncation (keep messages up to but excluding the selected user message) + let rewound_message_content = context.messages[full_idx] + .content() + .map(|s| clean_user_prompt(s)) + .unwrap_or_default(); - // Perform the truncation (keep messages up to and including the selected user message) let truncated_context = context.clone().truncate_to_user_message(keep_nth_user); let removed = total_before - truncated_context.messages.len(); let num_messages = truncated_context.messages.len(); @@ -2797,6 +2788,20 @@ impl A + Send + Sync> UI .sub_title(format!("[{}] {summary}", target_id.into_string())), )?; + // Set the rewound message content in the buffer for review/editing + if !rewound_message_content.is_empty() { + if self.cli.is_interactive() { + self.console.set_buffer(rewound_message_content); + } else { + // Check if we should write to a temporary file for the shell plugin + if let Ok(rewind_file) = std::env::var("FORGE_REWIND_FILE") { + let _ = std::fs::write(rewind_file, &rewound_message_content); + } + // Also print to stdout as fallback + println!("{rewound_message_content}"); + } + } + // If this is the active conversation, update the state if self.state.conversation_id == Some(target_id) { self.spinner.start(Some("Reloading conversation context"))?; diff --git a/crates/forge_services/src/tool_services/fs_write.rs b/crates/forge_services/src/tool_services/fs_write.rs index 976cbb750b..3b63628f52 100644 --- a/crates/forge_services/src/tool_services/fs_write.rs +++ b/crates/forge_services/src/tool_services/fs_write.rs @@ -95,10 +95,8 @@ impl< (None, default_ending) }; - // SNAPSHOT COORDINATION: Capture snapshot before writing if file exists - if file_exists { - self.infra.insert_snapshot(path).await?; - } + // SNAPSHOT COORDINATION: Capture snapshot before writing + self.infra.insert_snapshot(path).await?; // Normalize line endings to match the target style before writing let normalized_content = content diff --git a/crates/forge_services/src/tool_services/plan_create.rs b/crates/forge_services/src/tool_services/plan_create.rs index 81b9077c00..3aa788f661 100644 --- a/crates/forge_services/src/tool_services/plan_create.rs +++ b/crates/forge_services/src/tool_services/plan_create.rs @@ -7,6 +7,7 @@ use forge_app::{ EnvironmentInfra, FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileWriterInfra, PlanCreateOutput, PlanCreateService, }; +use forge_domain::SnapshotRepository; /// Creates a new plan file with the specified name, version, and content. Use /// this tool to create structured project plans, task breakdowns, or @@ -27,6 +28,7 @@ impl< + FileReaderInfra + FileWriterInfra + EnvironmentInfra + + SnapshotRepository + Send + Sync, > PlanCreateService for ForgePlanCreate @@ -65,12 +67,18 @@ impl< )); } + // SNAPSHOT COORDINATION: Capture snapshot before writing + self.0.insert_snapshot(&file_path).await?; + // Write the plan file self.0 .write(&file_path, Bytes::from(content)) .await .with_context(|| format!("Failed to write plan file: {}", file_path.display()))?; - Ok(PlanCreateOutput { path: file_path, before: None }) + Ok(PlanCreateOutput { + path: file_path, + before: None, + }) } } diff --git a/crates/forge_snaps/src/service.rs b/crates/forge_snaps/src/service.rs index 4fa11b53dc..c27b426342 100644 --- a/crates/forge_snaps/src/service.rs +++ b/crates/forge_snaps/src/service.rs @@ -28,9 +28,14 @@ impl SnapshotService { ForgeFS::create_dir_all(parent).await?; } - let content = ForgeFS::read(&snapshot.path).await?; - let path = snapshot.snapshot_path(Some(self.snapshots_directory.clone())); - ForgeFS::write(path, content).await?; + if ForgeFS::exists(&snapshot.path) { + let content = ForgeFS::read(&snapshot.path).await?; + ForgeFS::write(snapshot_path, content).await?; + } else { + // Write a special marker for "non-existent file" + let marker_path = snapshot_path.with_extension("none"); + ForgeFS::write(marker_path, b"").await?; + } Ok(snapshot) } @@ -43,7 +48,8 @@ impl SnapshotService { while let Some(entry) = dir.next_entry().await? { let filename = entry.file_name().to_string_lossy().to_string(); - if filename.ends_with(".snap") + // Match both .snap (regular) and .none (file didn't exist) + if (filename.ends_with(".snap") || filename.ends_with(".none")) && (latest_filename.is_none() || filename > latest_filename.clone().unwrap()) { latest_filename = Some(filename); @@ -70,9 +76,16 @@ impl SnapshotService { .await? .context(format!("No valid snapshots found for {path:?}"))?; - // Restore the content - let content = ForgeFS::read(&snapshot_path).await?; - ForgeFS::write(&path, content).await?; + if snapshot_path.extension().and_then(|e| e.to_str()) == Some("none") { + // The file didn't exist when snapshot was taken, so delete it if it exists now + if ForgeFS::exists(&path) { + ForgeFS::remove_file(&path).await?; + } + } else { + // Restore the content + let content = ForgeFS::read(&snapshot_path).await?; + ForgeFS::write(&path, content).await?; + } // Remove the used snapshot ForgeFS::remove_file(&snapshot_path).await?; @@ -208,6 +221,29 @@ mod tests { Ok(()) } + #[tokio::test] + async fn test_undo_file_creation() -> Result<()> { + // Arrange + let ctx = TestContext::new().await?; + let content = "New file content"; + + // Act + // 1. Snapshot non-existent file + ctx.service.create_snapshot(ctx.test_file.clone()).await?; + + // 2. Create the file + ctx.write_content(content).await?; + assert!(ForgeFS::exists(&ctx.test_file)); + + // 3. Undo creation + ctx.undo_snapshot().await?; + + // Assert + assert!(!ForgeFS::exists(&ctx.test_file)); + + Ok(()) + } + #[tokio::test] async fn test_multiple_snapshots() -> Result<()> { // Arrange diff --git a/shell-plugin/lib/actions/conversation.zsh b/shell-plugin/lib/actions/conversation.zsh index 4a31c8bbf1..d0365650ed 100644 --- a/shell-plugin/lib/actions/conversation.zsh +++ b/shell-plugin/lib/actions/conversation.zsh @@ -235,6 +235,43 @@ function _forge_action_conversation_rename() { fi } +# Action handler: Rewind conversation +# Usage: :rewind [] +function _forge_action_rewind() { + local input_text="$1" + local forge_dir=".forge" + + # Create .forge directory if it doesn't exist + if [[ ! -d "$forge_dir" ]]; then + mkdir -p "$forge_dir" || return 1 + fi + + local rewind_file="${forge_dir}/FORGE_REWIND_MSG" + rm -f "$rewind_file" + + # Use _forge_exec_interactive to allow the Rust TUI message picker + # We export FORGE_REWIND_FILE so the rust process knows where to write the content + local -x FORGE_REWIND_FILE="$rewind_file" + _forge_exec_interactive conversation rewind $input_text + + # Check if a message was rewound and we have content + if [[ -f "$rewind_file" ]]; then + local content + content=$(cat "$rewind_file" | tr -d '\r') + if [[ -n "$content" ]]; then + # Pre-populate buffer with the rewound message + # Prefix with : to indicate it's a forge command if the user wants to resubmit + # Actually, if it's a regular message, they might want to just type it. + # But usually in zsh mode they are typing : + BUFFER=": $content" + CURSOR=${#BUFFER} + fi + rm -f "$rewind_file" + fi + + # Note: caller (_forge_reset) will handle reset-prompt +} + # Helper function to clone and switch to conversation function _forge_clone_and_switch() { local clone_target="$1" diff --git a/shell-plugin/lib/dispatcher.zsh b/shell-plugin/lib/dispatcher.zsh index c5f13db0e0..a5ada11df2 100644 --- a/shell-plugin/lib/dispatcher.zsh +++ b/shell-plugin/lib/dispatcher.zsh @@ -241,6 +241,9 @@ function forge-accept-line() { conversation-rename) _forge_action_conversation_rename "$input_text" ;; + rewind) + _forge_action_rewind "$input_text" + ;; copy) _forge_action_copy ;; From 049461e8b847f82319ede9533d2ba663f73b45a0 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 02:33:22 +0530 Subject: [PATCH 12/18] chore: code formatting cleanup and fallback path extraction in modified_files_from - Formatting cleanup across forge_api, tool_registry, context, info, ui, and plan_create (line wrapping, closure simplification, whitespace removal). - Add fallback dynamic path extraction in modified_files_from by parsing XML tags from tool output, for backward compatibility with older conversations where modified_files may be empty. Co-Authored-By: ForgeCode --- crates/forge_api/src/forge_api.rs | 4 +- crates/forge_app/src/tool_registry.rs | 8 +-- crates/forge_domain/src/context.rs | 65 ++++++++++++++----- crates/forge_main/src/info.rs | 2 +- crates/forge_main/src/ui.rs | 30 +++++---- .../src/tool_services/plan_create.rs | 5 +- 6 files changed, 69 insertions(+), 45 deletions(-) diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index b145dfc7f9..49659e7c74 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -437,9 +437,7 @@ impl< } async fn undo_snapshot(&self, path: &str) -> Result<()> { - self.infra - .undo_snapshot(std::path::Path::new(path)) - .await?; + self.infra.undo_snapshot(std::path::Path::new(path)).await?; Ok(()) } diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index c634dcfbff..929f688b7a 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -5,8 +5,8 @@ use anyhow::Context; use console::style; use forge_domain::{ Agent, AgentId, AgentInput, ChatResponse, ChatResponseContent, Environment, InputModality, - Model, SystemContext, TemplateConfig, ToolCallContext, ToolCallFull, - ToolCatalog, ToolDefinition, ToolKind, ToolName, ToolOutput, ToolResult, + Model, SystemContext, TemplateConfig, ToolCallContext, ToolCallFull, ToolCatalog, + ToolDefinition, ToolKind, ToolName, ToolOutput, ToolResult, }; use forge_template::Element; use futures::future::join_all; @@ -429,9 +429,7 @@ impl ToolRegistry { match call.name.as_str() { "write" | "patch" | "multi_patch" | "remove" => { if let Ok(args) = call.arguments.parse() { - if let Some(file_path) = - args.get("file_path").and_then(|v| v.as_str()) - { + if let Some(file_path) = args.get("file_path").and_then(|v| v.as_str()) { return vec![file_path.to_string()]; } if let Some(path) = args.get("path").and_then(|v| v.as_str()) { diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index 8d2dfd8d06..628e97bbc2 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -708,13 +708,9 @@ impl Context { .messages .iter() .enumerate() - .filter_map(|(i, entry)| { - match &entry.message { - ContextMessage::Text(msg) if msg.role == Role::User && !msg.droppable => { - Some(i) - } - _ => None, - } + .filter_map(|(i, entry)| match &entry.message { + ContextMessage::Text(msg) if msg.role == Role::User && !msg.droppable => Some(i), + _ => None, }) .nth(nth_user); @@ -734,11 +730,9 @@ impl Context { self.messages .iter() .enumerate() - .filter(|(_, entry)| { - match &entry.message { - ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, - _ => false, - } + .filter(|(_, entry)| match &entry.message { + ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, + _ => false, }) .enumerate() .map(|(_user_idx, (full_idx, entry))| { @@ -760,11 +754,9 @@ impl Context { pub fn user_message_count(&self) -> usize { self.messages .iter() - .filter(|msg| { - match &msg.message { - ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, - _ => false, - } + .filter(|msg| match &msg.message { + ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, + _ => false, }) .count() } @@ -777,7 +769,41 @@ impl Context { if from_index < self.messages.len() { for msg in &self.messages[from_index..] { if let ContextMessage::Tool(result) = &msg.message { + // 1. Use the pre-calculated modified files from the tool execution files.extend(result.modified_files.iter().cloned()); + + // 2. Fallback dynamic extraction for older conversations where modified_files might be empty + if let Some(text) = result.output.as_str() { + let mut extracted_paths = Vec::new(); + if let Some(tag) = crate::xml::extract_tag(text, "plan_created") { + if let Some(path) = crate::xml::extract_attribute(tag, "path") { + extracted_paths.push(path); + } + } else if let Some(tag) = crate::xml::extract_tag(text, "file_created") { + if let Some(path) = crate::xml::extract_attribute(tag, "path") { + extracted_paths.push(path); + } + } else if let Some(tag) = crate::xml::extract_tag(text, "file_overwritten") + { + if let Some(path) = crate::xml::extract_attribute(tag, "path") { + extracted_paths.push(path); + } + } else if let Some(tag) = crate::xml::extract_tag(text, "file_diff") { + if let Some(path) = crate::xml::extract_attribute(tag, "path") { + extracted_paths.push(path); + } + } else if let Some(tag) = crate::xml::extract_tag(text, "file_removed") { + if let Some(path) = crate::xml::extract_attribute(tag, "path") { + extracted_paths.push(path); + } + } + + for p in extracted_paths { + if !files.contains(&p) { + files.push(p); + } + } + } } } } @@ -1850,7 +1876,10 @@ mod tests { // From U1 (idx 0) let files = context.modified_files_from(0); - assert_eq!(files, vec!["file1.txt".to_string(), "file2.txt".to_string()]); + assert_eq!( + files, + vec!["file1.txt".to_string(), "file2.txt".to_string()] + ); } /// Regression test: when both `reasoning` (raw text) and diff --git a/crates/forge_main/src/info.rs b/crates/forge_main/src/info.rs index b0815a8799..074e8e9711 100644 --- a/crates/forge_main/src/info.rs +++ b/crates/forge_main/src/info.rs @@ -75,7 +75,7 @@ impl Section { /// # Output Format /// /// ```text -/// +/// /// CONFIGURATION /// model gpt-4 /// provider openai diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 7c3076ca82..5aaf90eaca 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2691,28 +2691,27 @@ impl A + Send + Sync> UI let lines = context.format_messages_for_rewind(); let user_count = lines.len(); if user_count == 0 { - self.writeln_title(TitleFormat::error( - "No user messages to rewind to.", - ))?; + self.writeln_title(TitleFormat::error("No user messages to rewind to."))?; return Ok(()); } - let last_full_idx = lines.last().map(|(fi, _)| fi.to_string()).unwrap_or_default(); + let last_full_idx = lines + .last() + .map(|(fi, _)| fi.to_string()) + .unwrap_or_default(); let mut rows: Vec = Vec::with_capacity(lines.len()); for (full_idx, display) in &lines { rows.push( - SelectRow::new(full_idx.to_string(), display.clone()) - .search(display.clone()), + SelectRow::new(full_idx.to_string(), display.clone()).search(display.clone()), ); } - let selected = - tokio::task::spawn_blocking(move || -> anyhow::Result> { - Ok(ForgeWidget::select_rows("Rewind to message", rows) - .initial_raw(last_full_idx) - .prompt()?) - }) - .await??; + let selected = tokio::task::spawn_blocking(move || -> anyhow::Result> { + Ok(ForgeWidget::select_rows("Rewind to message", rows) + .initial_raw(last_full_idx) + .prompt()?) + }) + .await??; let full_idx = match selected { Some(row) => row @@ -2772,7 +2771,10 @@ impl A + Send + Sync> UI let status = if failed == 0 { format!("Reverted {total_undos} file change(s).") } else { - format!("Reverted {} of {total_undos} file change(s). {failed} failed.", total_undos - failed as usize) + format!( + "Reverted {} of {total_undos} file change(s). {failed} failed.", + total_undos - failed as usize + ) }; self.writeln(status)?; } diff --git a/crates/forge_services/src/tool_services/plan_create.rs b/crates/forge_services/src/tool_services/plan_create.rs index 3aa788f661..d9259f7f85 100644 --- a/crates/forge_services/src/tool_services/plan_create.rs +++ b/crates/forge_services/src/tool_services/plan_create.rs @@ -76,9 +76,6 @@ impl< .await .with_context(|| format!("Failed to write plan file: {}", file_path.display()))?; - Ok(PlanCreateOutput { - path: file_path, - before: None, - }) + Ok(PlanCreateOutput { path: file_path, before: None }) } } From 8fcf27230ec1234b9eb4e3740b686d9155efe968 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 11:36:47 +0530 Subject: [PATCH 13/18] refactor: extract shared logic and improve code reuse - Extract `resolve_conversation_id` in UI to eliminate duplicated conversation resolution logic between clone and rewind commands - Add `MessageEntry::is_user_message()` helper and use it across truncate, format, and count operations for DRYer context code - Introduce `extract_modified_files_from_output()` in xml module to centralize file path extraction from XML tags, and use it in both tool_registry and context - Use let-chains for more idiomatic Rust conditionals - Introduce `REWIND_PREVIEW_MAX_LEN` constant instead of magic number - Fix typo in test fixture ('receipe' -> 'recipe') - Add tests for `extract_tag_content` with duplicate closing tags and `extract_modified_files_from_output` Co-Authored-By: ForgeCode --- crates/forge_app/src/tool_registry.rs | 33 +------- crates/forge_domain/src/context.rs | 69 ++++++---------- crates/forge_domain/src/xml.rs | 76 ++++++++++++++--- crates/forge_main/src/ui.rs | 114 +++++++++++--------------- 4 files changed, 140 insertions(+), 152 deletions(-) diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index 929f688b7a..91f1f3d861 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -221,35 +221,10 @@ impl> ToolReg let output = self.call_inner(agent, call.clone(), context).await; let mut modified_files = Vec::new(); - match &output { - Ok(output) => { - if let Some(text) = output.as_str() { - // Extract path from plan_created or file_created tags - // These tags usually contain absolute paths - if let Some(tag) = forge_domain::extract_tag(text, "plan_created") { - if let Some(path) = forge_domain::extract_attribute(tag, "path") { - modified_files.push(path); - } - } else if let Some(tag) = forge_domain::extract_tag(text, "file_created") { - if let Some(path) = forge_domain::extract_attribute(tag, "path") { - modified_files.push(path); - } - } else if let Some(tag) = forge_domain::extract_tag(text, "file_overwritten") { - if let Some(path) = forge_domain::extract_attribute(tag, "path") { - modified_files.push(path); - } - } else if let Some(tag) = forge_domain::extract_tag(text, "file_diff") { - if let Some(path) = forge_domain::extract_attribute(tag, "path") { - modified_files.push(path); - } - } else if let Some(tag) = forge_domain::extract_tag(text, "file_removed") { - if let Some(path) = forge_domain::extract_attribute(tag, "path") { - modified_files.push(path); - } - } - } - } - _ => {} + if let Ok(output) = &output + && let Some(text) = output.as_str() + { + modified_files = forge_domain::extract_modified_files_from_output(text); } // Fallback to extraction from arguments if output didn't yield anything diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index 628e97bbc2..eaabf8e237 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -375,6 +375,17 @@ pub struct MessageEntry { pub usage: Option, } +impl MessageEntry { + /// Returns true if this is a user message that should be visible in history + /// (not droppable). + pub fn is_user_message(&self) -> bool { + match &self.message { + ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, + _ => false, + } + } +} + impl From for MessageEntry { fn from(value: ContextMessage) -> Self { MessageEntry { message: value, usage: Default::default() } @@ -433,6 +444,8 @@ pub struct Context { pub response_format: Option, } +const REWIND_PREVIEW_MAX_LEN: usize = 100; + impl Context { pub fn accumulate_usage(&self) -> Option { self.messages @@ -708,19 +721,13 @@ impl Context { .messages .iter() .enumerate() - .filter_map(|(i, entry)| match &entry.message { - ContextMessage::Text(msg) if msg.role == Role::User && !msg.droppable => Some(i), - _ => None, - }) + .filter_map(|(i, entry)| entry.is_user_message().then_some(i)) .nth(nth_user); - match cut_at { - Some(idx) if idx < self.messages.len() => { - self.messages.truncate(idx); - self - } - _ => self, + if let Some(idx) = cut_at { + self.messages.truncate(idx); } + self } /// Formats user messages with numbered indices for display during interactive @@ -730,18 +737,13 @@ impl Context { self.messages .iter() .enumerate() - .filter(|(_, entry)| match &entry.message { - ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, - _ => false, - }) - .enumerate() - .map(|(_user_idx, (full_idx, entry))| { + .filter(|(_, entry)| entry.is_user_message()) + .map(|(full_idx, entry)| { let content = entry.content().unwrap_or("").trim(); let cleaned = clean_user_prompt(content); let preview = strip_xml_tags(&cleaned); - let max_len = 100; - let preview = if preview.len() > max_len { - format!("{}...", &preview[..max_len]) + let preview = if preview.len() > REWIND_PREVIEW_MAX_LEN { + format!("{}...", &preview[..REWIND_PREVIEW_MAX_LEN]) } else { preview.to_string() }; @@ -754,10 +756,7 @@ impl Context { pub fn user_message_count(&self) -> usize { self.messages .iter() - .filter(|msg| match &msg.message { - ContextMessage::Text(msg) => msg.role == Role::User && !msg.droppable, - _ => false, - }) + .filter(|msg| msg.is_user_message()) .count() } @@ -774,29 +773,7 @@ impl Context { // 2. Fallback dynamic extraction for older conversations where modified_files might be empty if let Some(text) = result.output.as_str() { - let mut extracted_paths = Vec::new(); - if let Some(tag) = crate::xml::extract_tag(text, "plan_created") { - if let Some(path) = crate::xml::extract_attribute(tag, "path") { - extracted_paths.push(path); - } - } else if let Some(tag) = crate::xml::extract_tag(text, "file_created") { - if let Some(path) = crate::xml::extract_attribute(tag, "path") { - extracted_paths.push(path); - } - } else if let Some(tag) = crate::xml::extract_tag(text, "file_overwritten") - { - if let Some(path) = crate::xml::extract_attribute(tag, "path") { - extracted_paths.push(path); - } - } else if let Some(tag) = crate::xml::extract_tag(text, "file_diff") { - if let Some(path) = crate::xml::extract_attribute(tag, "path") { - extracted_paths.push(path); - } - } else if let Some(tag) = crate::xml::extract_tag(text, "file_removed") { - if let Some(path) = crate::xml::extract_attribute(tag, "path") { - extracted_paths.push(path); - } - } + let extracted_paths = crate::xml::extract_modified_files_from_output(text); for p in extracted_paths { if !files.contains(&p) { diff --git a/crates/forge_domain/src/xml.rs b/crates/forge_domain/src/xml.rs index ec483797d9..0258d107d5 100644 --- a/crates/forge_domain/src/xml.rs +++ b/crates/forge_domain/src/xml.rs @@ -1,10 +1,10 @@ /// Extracts a full XML tag (including brackets) by its name pub fn extract_tag<'a>(text: &'a str, tag_name: &str) -> Option<&'a str> { let opening_pattern = format!(r"<{tag_name}(?:\s[^>]*?)?>"); - if let Ok(regex) = regex::Regex::new(&opening_pattern) { - if let Some(mat) = regex.find(text) { - return Some(mat.as_str()); - } + if let Ok(regex) = regex::Regex::new(&opening_pattern) + && let Some(mat) = regex.find(text) + { + return Some(mat.as_str()); } None } @@ -92,10 +92,10 @@ pub fn clean_user_prompt(text: &str) -> String { /// Extracts the value of an attribute from an XML tag pub fn extract_attribute(tag: &str, attr_name: &str) -> Option { let pattern = format!(r#"{attr_name}="([^"]*)""#, attr_name = attr_name); - if let Ok(regex) = regex::Regex::new(&pattern) { - if let Some(captures) = regex.captures(tag) { - return captures.get(1).map(|m| m.as_str().to_string()); - } + if let Ok(regex) = regex::Regex::new(&pattern) + && let Some(captures) = regex.captures(tag) + { + return captures.get(1).map(|m| m.as_str().to_string()); } None } @@ -110,6 +110,30 @@ pub fn strip_xml_tags(text: &str) -> String { re_whitespace.replace_all(&result, " ").trim().to_string() } +/// Extracts file paths from XML tags in the given text. +/// Supports tags: plan_created, file_created, file_overwritten, file_diff, file_removed. +pub fn extract_modified_files_from_output(text: &str) -> Vec { + let mut modified_files = Vec::new(); + let tags = [ + "plan_created", + "file_created", + "file_overwritten", + "file_diff", + "file_removed", + ]; + + for tag_name in tags { + if let Some(tag) = extract_tag(text, tag_name) + && let Some(path) = extract_attribute(tag, "path") + { + modified_files.push(path); + // Return only the first matching tag to maintain parity with existing logic + return modified_files; + } + } + modified_files +} + #[cfg(test)] mod tests { use pretty_assertions::assert_eq; @@ -124,6 +148,14 @@ mod tests { assert_eq!(actual, expected); } + #[test] + fn test_extract_tag_content_with_duplicate_closing_tags() { + let fixture = "Some text 123 and more text"; + let actual = extract_tag_content(fixture, "summary"); + let expected = Some("123"); + assert_eq!(actual, expected); + } + #[test] fn test_extract_tag_content_no_tags() { let fixture = "Some text without any tags"; @@ -212,10 +244,10 @@ mod tests { #[test] fn test_clean_user_prompt_with_tags() { - let fixture = "add feature to determine receipe from images using vision llm models\n::: 2026-05-16\n::: "; + let fixture = "add feature to determine recipe from images using vision llm models\n::: 2026-05-16\n::: "; let actual = clean_user_prompt(fixture); // Should extract ONLY feedback and trim - let expected = "add feature to determine receipe from images using vision llm models"; + let expected = "add feature to determine recipe from images using vision llm models"; assert_eq!(actual, expected); } @@ -227,4 +259,28 @@ mod tests { let expected = "Just plain text"; assert_eq!(actual, expected); } + + #[test] + fn test_extract_modified_files_from_output() { + let fixture = r#"Some text more text"#; + let actual = extract_modified_files_from_output(fixture); + let expected = vec!["/abs/path/to/file.txt".to_string()]; + assert_eq!(actual, expected); + + let fixture = r#"Plan content"#; + let actual = extract_modified_files_from_output(fixture); + let expected = vec!["plan.md".to_string()]; + assert_eq!(actual, expected); + + let fixture = r#""#; + let actual = extract_modified_files_from_output(fixture); + // Priority: file_created > file_overwritten + let expected = vec!["new.txt".to_string()]; + assert_eq!(actual, expected); + + let fixture = "No tags here"; + let actual = extract_modified_files_from_output(fixture); + let expected: Vec = vec![]; + assert_eq!(actual, expected); + } } diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 5aaf90eaca..a81c072865 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -118,6 +118,46 @@ pub struct UI A> { } impl A + Send + Sync> UI { + /// Resolves a conversation ID from an optional string ID, the current + /// active conversation, or by showing an interactive picker. + async fn resolve_conversation_id( + &mut self, + id: Option, + ) -> anyhow::Result> { + if let Some(id_str) = id { + return Ok(Some( + ConversationId::parse(&id_str) + .map_err(|_| anyhow::anyhow!("Invalid conversation ID: {id_str}"))?, + )); + } + + if let Some(cid) = self.state.conversation_id { + return Ok(Some(cid)); + } + + // Show conversation picker + let conversations = self + .api + .get_conversations(Some(self.config.max_conversations)) + .await?; + + if conversations.is_empty() { + self.writeln_title(TitleFormat::error( + "No conversations found. Start a conversation first.", + ))?; + return Ok(None); + } + + let selected = ConversationSelector::select_conversation( + &conversations, + self.state.conversation_id, + None, + ) + .await?; + + Ok(selected.map(|conv| conv.id)) + } + /// Writes a line to the console output /// Takes anything that implements ToString trait fn writeln(&mut self, content: T) -> anyhow::Result<()> { @@ -2574,34 +2614,8 @@ impl A + Send + Sync> UI /// conversation is used; if no active conversation, an interactive picker /// is shown. async fn on_slash_clone(&mut self, id: Option) -> anyhow::Result<()> { - let target_id = if let Some(id_str) = id { - ConversationId::parse(&id_str) - .map_err(|_| anyhow::anyhow!("Invalid conversation ID: {id_str}"))? - } else { - // Show conversation picker - let conversations = self - .api - .get_conversations(Some(self.config.max_conversations)) - .await?; - - if conversations.is_empty() { - self.writeln_title(TitleFormat::error( - "No conversations found. Start a conversation first.", - ))?; - return Ok(()); - } - - let selected = ConversationSelector::select_conversation( - &conversations, - self.state.conversation_id, - None, - ) - .await?; - - match selected { - Some(conv) => conv.id, - None => return Ok(()), - } + let Some(target_id) = self.resolve_conversation_id(id).await? else { + return Ok(()); }; // Fetch the conversation to clone @@ -2636,36 +2650,8 @@ impl A + Send + Sync> UI /// * `id` - Optional conversation ID. If `None`, an interactive picker is /// shown. async fn on_slash_rewind(&mut self, id: Option) -> anyhow::Result<()> { - let target_id = if let Some(id_str) = id { - ConversationId::parse(&id_str) - .map_err(|_| anyhow::anyhow!("Invalid conversation ID: {id_str}"))? - } else if let Some(cid) = self.state.conversation_id { - cid - } else { - // Show conversation picker - let conversations = self - .api - .get_conversations(Some(self.config.max_conversations)) - .await?; - - if conversations.is_empty() { - self.writeln_title(TitleFormat::error( - "No conversations found. Start a conversation first.", - ))?; - return Ok(()); - } - - let selected = ConversationSelector::select_conversation( - &conversations, - self.state.conversation_id, - None, - ) - .await?; - - match selected { - Some(conv) => conv.id, - None => return Ok(()), - } + let Some(target_id) = self.resolve_conversation_id(id).await? else { + return Ok(()); }; // Fetch the conversation @@ -2707,9 +2693,9 @@ impl A + Send + Sync> UI } let selected = tokio::task::spawn_blocking(move || -> anyhow::Result> { - Ok(ForgeWidget::select_rows("Rewind to message", rows) + ForgeWidget::select_rows("Rewind to message", rows) .initial_raw(last_full_idx) - .prompt()?) + .prompt() }) .await??; @@ -2740,7 +2726,7 @@ impl A + Send + Sync> UI // Perform the truncation (keep messages up to but excluding the selected user message) let rewound_message_content = context.messages[full_idx] .content() - .map(|s| clean_user_prompt(s)) + .map(clean_user_prompt) .unwrap_or_default(); let truncated_context = context.clone().truncate_to_user_message(keep_nth_user); @@ -2804,12 +2790,6 @@ impl A + Send + Sync> UI } } - // If this is the active conversation, update the state - if self.state.conversation_id == Some(target_id) { - self.spinner.start(Some("Reloading conversation context"))?; - self.spinner.stop(None)?; - } - Ok(()) } From c73e1ec9df90114a39497e4d579a95d4d570e211 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 12:38:39 +0530 Subject: [PATCH 14/18] feat: improve task tag handling, conversation picker, and shell plugin - Add support for tag extraction in prompt cleaning (xml.rs) - Strip terminal_context tags from prompts to prevent clutter - Skip conversation picker when --conversation-id is provided (ui.rs) - Make :rewind and :clone fall back to current conversation ID when no target is given (conversation.zsh) --- crates/forge_domain/src/xml.rs | 22 +++++++++++++++++++++- crates/forge_main/src/ui.rs | 4 ++++ shell-plugin/lib/actions/conversation.zsh | 22 +++++++++++++++++++--- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/crates/forge_domain/src/xml.rs b/crates/forge_domain/src/xml.rs index 0258d107d5..622ec03cbd 100644 --- a/crates/forge_domain/src/xml.rs +++ b/crates/forge_domain/src/xml.rs @@ -72,14 +72,18 @@ pub fn remove_tag_with_prefix(text: &str, prefix: &str) -> String { /// Cleans a user prompt by extracting content from tags if present, /// or stripping all XML tags and meta-information. pub fn clean_user_prompt(text: &str) -> String { - // 1. Try to extract content from tag + // 1. Try to extract content from or tags if let Some(content) = extract_tag_content(text, "feedback") { return content.to_string(); } + if let Some(content) = extract_tag_content(text, "task") { + return content.to_string(); + } // 2. Remove known meta tags with their content let mut cleaned = remove_tag_with_prefix(text, "system_"); cleaned = remove_tag_with_prefix(&cleaned, "context_"); + cleaned = remove_tag_with_prefix(&cleaned, "terminal_context"); // 3. Strip all remaining tags but preserve newlines (unlike strip_xml_tags) let tag_pattern = regex::Regex::new(r"<[^>]*>").unwrap(); @@ -260,6 +264,22 @@ mod tests { assert_eq!(actual, expected); } + #[test] + fn test_clean_user_prompt_with_terminal_context() { + let fixture = "create a planls"; + let actual = clean_user_prompt(fixture); + let expected = "create a plan"; + assert_eq!(actual, expected); + } + + #[test] + fn test_clean_user_prompt_with_task_only() { + let fixture = "new tasksome info"; + let actual = clean_user_prompt(fixture); + let expected = "new task"; + assert_eq!(actual, expected); + } + #[test] fn test_extract_modified_files_from_output() { let fixture = r#"Some text more text"#; diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index a81c072865..86cdfb862b 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -135,6 +135,10 @@ impl A + Send + Sync> UI return Ok(Some(cid)); } + if let Some(cid) = self.cli.conversation_id { + return Ok(Some(cid)); + } + // Show conversation picker let conversations = self .api diff --git a/shell-plugin/lib/actions/conversation.zsh b/shell-plugin/lib/actions/conversation.zsh index d0365650ed..2439436ae7 100644 --- a/shell-plugin/lib/actions/conversation.zsh +++ b/shell-plugin/lib/actions/conversation.zsh @@ -252,7 +252,13 @@ function _forge_action_rewind() { # Use _forge_exec_interactive to allow the Rust TUI message picker # We export FORGE_REWIND_FILE so the rust process knows where to write the content local -x FORGE_REWIND_FILE="$rewind_file" - _forge_exec_interactive conversation rewind $input_text + + local target_id="$input_text" + if [[ -z "$target_id" ]]; then + target_id="$_FORGE_CONVERSATION_ID" + fi + + _forge_exec_interactive conversation rewind $target_id # Check if a message was rewound and we have content if [[ -f "$rewind_file" ]]; then @@ -276,13 +282,23 @@ function _forge_action_rewind() { function _forge_clone_and_switch() { local clone_target="$1" + local target_id="$clone_target" + if [[ -z "$target_id" ]]; then + target_id="$_FORGE_CONVERSATION_ID" + fi + # Store original conversation ID to check if we're cloning current conversation local original_conversation_id="$_FORGE_CONVERSATION_ID" # Execute clone command - _forge_log info "Cloning conversation \033[1m${clone_target}\033[0m" + if [[ -n "$target_id" ]]; then + _forge_log info "Cloning conversation \033[1m${target_id}\033[0m" + else + _forge_log info "Cloning conversation" + fi + local clone_output - clone_output=$($_FORGE_BIN conversation clone "$clone_target" 2>&1) + clone_output=$($_FORGE_BIN conversation clone "$target_id" 2>&1) local clone_exit_code=$? if [[ $clone_exit_code -eq 0 ]]; then From e17824aa39db0d6703c0d460c577a9cfcec0c893 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 13:02:25 +0530 Subject: [PATCH 15/18] fix: improve rewind prompt reset and dispatcher integration - Call zle reset-prompt directly in rewind action instead of relying on caller to handle it (conversation.zsh) - Add OSC133 status emission for rewind action in the dispatcher to ensure proper terminal state management (dispatcher.zsh) --- shell-plugin/lib/actions/conversation.zsh | 2 +- shell-plugin/lib/dispatcher.zsh | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/shell-plugin/lib/actions/conversation.zsh b/shell-plugin/lib/actions/conversation.zsh index 2439436ae7..e48483389a 100644 --- a/shell-plugin/lib/actions/conversation.zsh +++ b/shell-plugin/lib/actions/conversation.zsh @@ -275,7 +275,7 @@ function _forge_action_rewind() { rm -f "$rewind_file" fi - # Note: caller (_forge_reset) will handle reset-prompt + zle reset-prompt } # Helper function to clone and switch to conversation diff --git a/shell-plugin/lib/dispatcher.zsh b/shell-plugin/lib/dispatcher.zsh index a5ada11df2..9eee4bc00f 100644 --- a/shell-plugin/lib/dispatcher.zsh +++ b/shell-plugin/lib/dispatcher.zsh @@ -243,6 +243,11 @@ function forge-accept-line() { ;; rewind) _forge_action_rewind "$input_text" + local action_status=$? + _forge_osc133_emit "D;$action_status" + _forge_osc133_emit "A" + # Note: rewind action intentionally modifies BUFFER and handles its own prompt reset + return $action_status ;; copy) _forge_action_copy From 2e42a97714f981c06040f6be2e794bbb70568940 Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 13:16:02 +0530 Subject: [PATCH 16/18] fix: correct modified files tracking scoping and dedup - Move modified_files extraction and fallback logic inside the Ok(output) block to prevent false positives from error results (tool_registry.rs) - Fix dedup check to use msg_modified instead of files to prevent cross-tool de-duplication of the same file (context.rs) - Add tests for duplicate modified files across tools and dedup within a single tool result (context.rs) --- crates/forge_app/src/tool_registry.rs | 18 +++++----- crates/forge_domain/src/context.rs | 50 +++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index 91f1f3d861..1a162df0bb 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -221,16 +221,16 @@ impl> ToolReg let output = self.call_inner(agent, call.clone(), context).await; let mut modified_files = Vec::new(); - if let Ok(output) = &output - && let Some(text) = output.as_str() - { - modified_files = forge_domain::extract_modified_files_from_output(text); - } + if let Ok(output) = &output { + if let Some(text) = output.as_str() { + modified_files = forge_domain::extract_modified_files_from_output(text); + } - // Fallback to extraction from arguments if output didn't yield anything - // This is important for relative paths provided in tool arguments - if modified_files.is_empty() { - modified_files = Self::extract_modified_files(&call); + // Fallback to extraction from arguments if output didn't yield anything + // This is important for relative paths provided in tool arguments + if modified_files.is_empty() { + modified_files = Self::extract_modified_files(&call); + } } ToolResult::new(tool_name) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index eaabf8e237..b0dea6162b 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -769,18 +769,20 @@ impl Context { for msg in &self.messages[from_index..] { if let ContextMessage::Tool(result) = &msg.message { // 1. Use the pre-calculated modified files from the tool execution - files.extend(result.modified_files.iter().cloned()); + let mut msg_modified = result.modified_files.clone(); // 2. Fallback dynamic extraction for older conversations where modified_files might be empty if let Some(text) = result.output.as_str() { let extracted_paths = crate::xml::extract_modified_files_from_output(text); for p in extracted_paths { - if !files.contains(&p) { - files.push(p); + if !msg_modified.contains(&p) { + msg_modified.push(p); } } } + + files.extend(msg_modified); } } } @@ -1859,6 +1861,48 @@ mod tests { ); } + #[test] + fn test_modified_files_from_duplicates() { + use crate::{ToolName, ToolOutput, ToolResult}; + let context = Context::default() + .add_message(ContextMessage::Tool(ToolResult { + name: ToolName::new("write"), + call_id: None, + output: ToolOutput::text("ok"), + modified_files: vec!["file1.txt".to_string()], + })) + .add_message(ContextMessage::Tool(ToolResult { + name: ToolName::new("patch"), + call_id: None, + output: ToolOutput::text("ok"), + modified_files: vec!["file1.txt".to_string()], + })); + + let files = context.modified_files_from(0); + // Should contain duplicates as each is a separate modification + assert_eq!( + files, + vec!["file1.txt".to_string(), "file1.txt".to_string()] + ); + } + + #[test] + fn test_modified_files_from_fallback_dedup() { + use crate::{ToolName, ToolOutput, ToolResult}; + let context = Context::default().add_message(ContextMessage::Tool(ToolResult { + name: ToolName::new("write"), + call_id: None, + // XML tag suggests file1.txt was modified + output: ToolOutput::text(""), + // result.modified_files ALSO has file1.txt + modified_files: vec!["file1.txt".to_string()], + })); + + let files = context.modified_files_from(0); + // Should NOT duplicate within the same tool result + assert_eq!(files, vec!["file1.txt".to_string()]); + } + /// Regression test: when both `reasoning` (raw text) and /// `reasoning_details` (structured, with a cryptographic signature) are /// present, `append_message` must NOT create a duplicate thinking block From 0181f48e94c18406b7a104f28fcfd846b433ac7c Mon Sep 17 00:00:00 2001 From: Rohith Mahesh Date: Sat, 16 May 2026 13:40:51 +0530 Subject: [PATCH 17/18] fix: address nightly clippy violations for string_slice and indexing_slicing - Use safe char-based truncation for rewind preview (context.rs:746) - Use .iter().skip() instead of range slicing in modified_files_from (context.rs:769) - Use .get() instead of direct indexing in rewind handler (ui.rs:2731) --- crates/forge_domain/src/context.rs | 9 +++++---- crates/forge_main/src/ui.rs | 6 ++++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index b0dea6162b..025076c33e 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -743,7 +743,10 @@ impl Context { let cleaned = clean_user_prompt(content); let preview = strip_xml_tags(&cleaned); let preview = if preview.len() > REWIND_PREVIEW_MAX_LEN { - format!("{}...", &preview[..REWIND_PREVIEW_MAX_LEN]) + format!( + "{}...", + preview.chars().take(REWIND_PREVIEW_MAX_LEN).collect::() + ) } else { preview.to_string() }; @@ -765,8 +768,7 @@ impl Context { /// file snapshots to revert. pub fn modified_files_from(&self, from_index: usize) -> Vec { let mut files = Vec::new(); - if from_index < self.messages.len() { - for msg in &self.messages[from_index..] { + for msg in self.messages.iter().skip(from_index) { if let ContextMessage::Tool(result) = &msg.message { // 1. Use the pre-calculated modified files from the tool execution let mut msg_modified = result.modified_files.clone(); @@ -785,7 +787,6 @@ impl Context { files.extend(msg_modified); } } - } files } diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 86cdfb862b..2aeb2a19b1 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2728,8 +2728,10 @@ impl A + Send + Sync> UI let modified_files = context.modified_files_from(full_idx); // Perform the truncation (keep messages up to but excluding the selected user message) - let rewound_message_content = context.messages[full_idx] - .content() + let rewound_message_content = context + .messages + .get(full_idx) + .and_then(|m| m.content()) .map(clean_user_prompt) .unwrap_or_default(); From f0a8410ae63aa6bec0db56bc24a3a4068f26ebb2 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 16 May 2026 08:13:16 +0000 Subject: [PATCH 18/18] [autofix.ci] apply automated fixes --- crates/forge_domain/src/context.rs | 41 +++++++++++++++++------------- crates/forge_domain/src/xml.rs | 7 ++--- crates/forge_main/src/info.rs | 2 +- crates/forge_main/src/ui.rs | 13 +++++----- 4 files changed, 35 insertions(+), 28 deletions(-) diff --git a/crates/forge_domain/src/context.rs b/crates/forge_domain/src/context.rs index 025076c33e..0fd9ec6b09 100644 --- a/crates/forge_domain/src/context.rs +++ b/crates/forge_domain/src/context.rs @@ -730,9 +730,10 @@ impl Context { self } - /// Formats user messages with numbered indices for display during interactive - /// rewind. Returns a list of `(full_message_index, display_string)` tuples, - /// where the display string shows a 1-indexed user message number and preview. + /// Formats user messages with numbered indices for display during + /// interactive rewind. Returns a list of `(full_message_index, + /// display_string)` tuples, where the display string shows a 1-indexed + /// user message number and preview. pub fn format_messages_for_rewind(&self) -> Vec<(usize, String)> { self.messages .iter() @@ -745,7 +746,10 @@ impl Context { let preview = if preview.len() > REWIND_PREVIEW_MAX_LEN { format!( "{}...", - preview.chars().take(REWIND_PREVIEW_MAX_LEN).collect::() + preview + .chars() + .take(REWIND_PREVIEW_MAX_LEN) + .collect::() ) } else { preview.to_string() @@ -769,24 +773,25 @@ impl Context { pub fn modified_files_from(&self, from_index: usize) -> Vec { let mut files = Vec::new(); for msg in self.messages.iter().skip(from_index) { - if let ContextMessage::Tool(result) = &msg.message { - // 1. Use the pre-calculated modified files from the tool execution - let mut msg_modified = result.modified_files.clone(); - - // 2. Fallback dynamic extraction for older conversations where modified_files might be empty - if let Some(text) = result.output.as_str() { - let extracted_paths = crate::xml::extract_modified_files_from_output(text); - - for p in extracted_paths { - if !msg_modified.contains(&p) { - msg_modified.push(p); - } + if let ContextMessage::Tool(result) = &msg.message { + // 1. Use the pre-calculated modified files from the tool execution + let mut msg_modified = result.modified_files.clone(); + + // 2. Fallback dynamic extraction for older conversations where modified_files + // might be empty + if let Some(text) = result.output.as_str() { + let extracted_paths = crate::xml::extract_modified_files_from_output(text); + + for p in extracted_paths { + if !msg_modified.contains(&p) { + msg_modified.push(p); } } - - files.extend(msg_modified); } + + files.extend(msg_modified); } + } files } diff --git a/crates/forge_domain/src/xml.rs b/crates/forge_domain/src/xml.rs index 622ec03cbd..31363ed35c 100644 --- a/crates/forge_domain/src/xml.rs +++ b/crates/forge_domain/src/xml.rs @@ -104,8 +104,8 @@ pub fn extract_attribute(tag: &str, attr_name: &str) -> Option { None } -/// Removes all XML/HTML tags from the text, keeping only the content between tags. -/// Multiple whitespace characters are collapsed into a single space. +/// Removes all XML/HTML tags from the text, keeping only the content between +/// tags. Multiple whitespace characters are collapsed into a single space. pub fn strip_xml_tags(text: &str) -> String { let tag_pattern = regex::Regex::new(r"<[^>]*>").unwrap(); let result = tag_pattern.replace_all(text, "").to_string(); @@ -115,7 +115,8 @@ pub fn strip_xml_tags(text: &str) -> String { } /// Extracts file paths from XML tags in the given text. -/// Supports tags: plan_created, file_created, file_overwritten, file_diff, file_removed. +/// Supports tags: plan_created, file_created, file_overwritten, file_diff, +/// file_removed. pub fn extract_modified_files_from_output(text: &str) -> Vec { let mut modified_files = Vec::new(); let tags = [ diff --git a/crates/forge_main/src/info.rs b/crates/forge_main/src/info.rs index 074e8e9711..b0815a8799 100644 --- a/crates/forge_main/src/info.rs +++ b/crates/forge_main/src/info.rs @@ -75,7 +75,7 @@ impl Section { /// # Output Format /// /// ```text -/// +/// /// CONFIGURATION /// model gpt-4 /// provider openai diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 2aeb2a19b1..c9c5b65a0b 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -125,10 +125,9 @@ impl A + Send + Sync> UI id: Option, ) -> anyhow::Result> { if let Some(id_str) = id { - return Ok(Some( - ConversationId::parse(&id_str) - .map_err(|_| anyhow::anyhow!("Invalid conversation ID: {id_str}"))?, - )); + return Ok(Some(ConversationId::parse(&id_str).map_err(|_| { + anyhow::anyhow!("Invalid conversation ID: {id_str}") + })?)); } if let Some(cid) = self.state.conversation_id { @@ -2724,10 +2723,12 @@ impl A + Send + Sync> UI // Collect file paths that were modified by tool results in messages // that will be removed. These files' snapshots need to be reverted. - // We use full_idx because it's the index of the user message we are rewinding AT. + // We use full_idx because it's the index of the user message we are rewinding + // AT. let modified_files = context.modified_files_from(full_idx); - // Perform the truncation (keep messages up to but excluding the selected user message) + // Perform the truncation (keep messages up to but excluding the selected user + // message) let rewound_message_content = context .messages .get(full_idx)