fix chat help (#1830)

Co-authored-by: Brandon Kiser <[email protected]>
Co-authored-by: Felix Ding <[email protected]>

Author: 3 people

crates/chat-cli/src/cli/chat/cli.rs

@@ -17,9 +17,9 @@ pub struct Chat {
     /// prompt requests permissions to use a tool, unless --trust-all-tools is also used.
     #[arg(long)]
     pub no_interactive: bool,
-    /// Start a new conversation and overwrites any previous conversation from this directory.
-    #[arg(long)]
-    pub new: bool,
+    /// Resumes the previous conversation from this directory.
+    #[arg(short, long)]
+    pub resume: bool,
     /// The first question to ask
     pub input: Option<String>,
     /// Context profile to use

crates/chat-cli/src/cli/chat/command.rs

@@ -51,10 +51,10 @@ pub enum Command {
         subcommand: Option<PromptsSubcommand>,
     },
     Usage,
-    Import {
+    Load {
         path: String,
     },
-    Export {
+    Save {
         path: String,
         force: bool,
     },
@@ -818,15 +818,15 @@ impl Command {
                     }
                 },
                 "usage" => Self::Usage,
-                "import" => {
+                "load" => {
                     let Some(path) = parts.get(1) else {
                         return Err("path is required".to_string());
                     };
-                    Self::Import {
+                    Self::Load {
                         path: (*path).to_string(),
                     }
                 },
-                "export" => {
+                "save" => {
                     let force = parts.contains(&"-f") || parts.contains(&"--force");
                     let Some(path) = parts.get(1) else {
                         return Err("path is required".to_string());
@@ -835,7 +835,7 @@ impl Command {
                     if !path.ends_with(".json") {
                         path.push_str(".json");
                     }
-                    Self::Export { path, force }
+                    Self::Save { path, force }
                 },
                 unknown_command => {
                     let looks_like_path = {

crates/chat-cli/src/cli/chat/conversation_state.rs

@@ -1,5 +1,6 @@
 use std::collections::{
     HashMap,
+    HashSet,
     VecDeque,
 };
 use std::sync::Arc;
@@ -32,7 +33,6 @@ use super::hooks::{
 };
 use super::message::{
     AssistantMessage,
-    AssistantToolUse,
     ToolUseResult,
     ToolUseResultBlock,
     UserMessage,
@@ -60,7 +60,6 @@ use crate::api_client::model::{
     ToolInputSchema,
     ToolResult,
     ToolResultContentBlock,
-    ToolResultStatus,
     ToolSpecification,
     ToolUse,
     UserInputMessage,
@@ -347,7 +346,7 @@ impl ConversationState {
             }
         }
 
-        self.enforce_tool_use_history_invariants(true);
+        self.enforce_tool_use_history_invariants();
     }
 
     /// Here we also need to make sure that the tool result corresponds to one of the tools
@@ -362,105 +361,52 @@ impl ConversationState {
     ///    intervention here is to substitute the ambiguous, partial name with a dummy.
     /// 3. The model had decided to call a tool that does not exist. The intervention here is to
     ///    substitute the non-existent tool name with a dummy.
-    pub fn enforce_tool_use_history_invariants(&mut self, last_only: bool) {
-        let tool_name_list = self.tool_manager.tn_map.keys().map(String::as_str).collect::<Vec<_>>();
-        // We need to first determine what the range of interest is. There are two places where we
-        // would call this function:
-        // 1. When there are changes to the list of available tools, in which case we comb through the
-        //    entire conversation
-        // 2. When we send a message, in which case we only examine the most recent entry
-        let (tool_use_results, mut tool_uses) = if last_only {
-            if let (Some((_, AssistantMessage::ToolUse { ref mut tool_uses, .. })), Some(user_msg)) = (
-                self.history
-                    .range_mut(self.valid_history_range.0..self.valid_history_range.1)
-                    .last(),
-                &mut self.next_message,
-            ) {
-                let tool_use_results = user_msg
-                    .tool_use_results()
-                    .map_or(Vec::new(), |results| results.iter().collect::<Vec<_>>());
-                let tool_uses = tool_uses.iter_mut().collect::<Vec<_>>();
-                (tool_use_results, tool_uses)
-            } else {
-                (Vec::new(), Vec::new())
-            }
-        } else {
-            let tool_use_results = self.next_message.as_ref().map_or(Vec::new(), |user_msg| {
-                user_msg
-                    .tool_use_results()
-                    .map_or(Vec::new(), |results| results.iter().collect::<Vec<_>>())
-            });
-            self.history
-                .iter_mut()
-                .filter_map(|(user_msg, asst_msg)| {
-                    if let (Some(tool_use_results), AssistantMessage::ToolUse { ref mut tool_uses, .. }) =
-                        (user_msg.tool_use_results(), asst_msg)
-                    {
-                        Some((tool_use_results, tool_uses))
-                    } else {
-                        None
-                    }
+    pub fn enforce_tool_use_history_invariants(&mut self) {
+        let tool_names: HashSet<_> = self
+            .tools
+            .values()
+            .flat_map(|tools| {
+                tools.iter().map(|tool| match tool {
+                    Tool::ToolSpecification(tool_specification) => tool_specification.name.as_str(),
                 })
-                .fold(
-                    (tool_use_results, Vec::<&mut AssistantToolUse>::new()),
-                    |(mut tool_use_results, mut tool_uses), (results, uses)| {
-                        let mut results = results.iter().collect::<Vec<_>>();
-                        let mut uses = uses.iter_mut().collect::<Vec<_>>();
-                        tool_use_results.append(&mut results);
-                        tool_uses.append(&mut uses);
-                        (tool_use_results, tool_uses)
-                    },
-                )
-        };
+            })
+            .filter(|name| *name != DUMMY_TOOL_NAME)
+            .collect();
+
+        for (_, assistant) in &mut self.history {
+            if let AssistantMessage::ToolUse { ref mut tool_uses, .. } = assistant {
+                for tool_use in tool_uses {
+                    if tool_names.contains(tool_use.name.as_str()) {
+                        continue;
+                    }
 
-        // Replace tool uses associated with tools that does not exist / no longer exists with
-        // dummy (i.e. put them to sleep / dormant)
-        for result in tool_use_results {
-            let tool_use_id = result.tool_use_id.as_str();
-            let corresponding_tool_use = tool_uses.iter_mut().find(|tool_use| tool_use_id == tool_use.id);
-            if let Some(tool_use) = corresponding_tool_use {
-                if tool_name_list.contains(&tool_use.name.as_str()) {
-                    // If this tool matches of the tools in our list, this is not our
-                    // concern, error or not.
-                    continue;
-                }
-                if let ToolResultStatus::Error = result.status {
-                    // case 2 and 3
-                    tool_use.name = DUMMY_TOOL_NAME.to_string();
-                    tool_use.args = serde_json::json!({});
-                } else {
-                    // case 1
-                    let full_name = tool_name_list.iter().find(|name| name.ends_with(&tool_use.name));
-                    // We should be able to find a match but if not we'll just treat it as
-                    // a dummy and move on
-                    if let Some(full_name) = full_name {
-                        tool_use.name = (*full_name).to_string();
-                    } else {
-                        tool_use.name = DUMMY_TOOL_NAME.to_string();
-                        tool_use.args = serde_json::json!({});
+                    if tool_names.contains(tool_use.orig_name.as_str()) {
+                        tool_use.name = tool_use.orig_name.clone();
+                        tool_use.args = tool_use.orig_args.clone();
+                        continue;
                     }
-                }
-            }
-        }
 
-        // Revive tools that were previously dormant if they now corresponds to one of the tools in
-        // our list of available tools. Note that this check only works because tn_map does NOT
-        // contain names of native tools.
-        for tool_use in tool_uses {
-            if tool_use.name == DUMMY_TOOL_NAME
-                && tool_use
-                    .orig_name
-                    .as_ref()
-                    .is_some_and(|name| tool_name_list.contains(&(*name).as_str()))
-            {
-                tool_use.name = tool_use
-                    .orig_name
-                    .as_ref()
-                    .map_or(DUMMY_TOOL_NAME.to_string(), |name| name.clone());
-                tool_use.args = tool_use
-                    .orig_args
-                    .as_ref()
-                    .map_or(serde_json::json!({}), |args| args.clone());
+                    let names: Vec<&str> = tool_names
+                        .iter()
+                        .filter_map(|name| {
+                            if name.ends_with(&tool_use.name) {
+                                Some(*name)
+                            } else {
+                                None
+                            }
+                        })
+                        .collect();
+
+                    // There's only one tool use matching, so we can just replace it with the
+                    // found name.
+                    if names.len() == 1 {
+                        tool_use.name = (*names.first().unwrap()).to_string();
+                        continue;
+                    }
+
+                    // Otherwise, we have to replace it with a dummy.
+                    tool_use.name = DUMMY_TOOL_NAME.to_string();
+                }
             }
         }
     }
@@ -514,8 +460,8 @@ impl ConversationState {
             .expect("unable to construct conversation state")
     }
 
-    pub async fn update_state(&mut self) {
-        let needs_update = self.tool_manager.has_new_stuff.load(Ordering::Acquire);
+    pub async fn update_state(&mut self, force_update: bool) {
+        let needs_update = self.tool_manager.has_new_stuff.load(Ordering::Acquire) || force_update;
         if !needs_update {
             return;
         }
@@ -540,12 +486,13 @@ impl ConversationState {
         // We call this in [Self::enforce_conversation_invariants] as well. But we need to call it
         // here as well because when it's being called in [Self::enforce_conversation_invariants]
         // it is only checking the last entry.
-        self.enforce_tool_use_history_invariants(false);
+        self.enforce_tool_use_history_invariants();
     }
 
     /// Returns a conversation state representation which reflects the exact conversation to send
     /// back to the model.
     pub async fn backend_conversation_state(&mut self, run_hooks: bool, quiet: bool) -> BackendConversationState<'_> {
+        self.update_state(false).await;
         self.enforce_conversation_invariants();
 
         // Run hooks and add to conversation start and next user message.

crates/chat-cli/src/cli/chat/message.rs

@@ -349,11 +349,11 @@ pub struct AssistantToolUse {
     /// The name for the tool as exposed to the model
     pub name: String,
     /// Original name of the tool
-    pub orig_name: Option<String>,
+    pub orig_name: String,
     /// The input to pass to the tool as exposed to the model
     pub args: serde_json::Value,
     /// Original input passed to the tool
-    pub orig_args: Option<serde_json::Value>,
+    pub orig_args: serde_json::Value,
 }
 
 impl From<AssistantToolUse> for ToolUse {