fix requires_acceptance for subcommands

Author: jsamuel1

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

@@ -1090,7 +1090,20 @@ impl Command {
         }
     }
 
-    /// Parse a command from components (for use by internal_command tool)
+    /// Parse a command from components
+    ///
+    /// This method formats a command string from its components and parses it into a Command enum.
+    ///
+    /// # Arguments
+    ///
+    /// * `command` - The base command name
+    /// * `subcommand` - Optional subcommand
+    /// * `args` - Optional arguments
+    /// * `flags` - Optional flags
+    ///
+    /// # Returns
+    ///
+    /// * `Result<Self>` - The parsed Command enum
     pub fn parse_from_components(
         command: &str,
         subcommand: Option<&String>,

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

@@ -1339,8 +1339,8 @@ impl ChatContext {
         if let Some(index) = pending_tool_index {
             let tool_use = &mut tool_uses[index];
 
-            let is_trust = ["t", "T"].contains(&prompt.as_str());
-            if ["y", "Y"].contains(&prompt.as_str()) || is_trust {
+            let is_trust = ["t", "T"].contains(&prompt);
+            if ["y", "Y"].contains(&prompt) || is_trust {
                 if is_trust {
                     self.tool_permissions.trust_tool(&tool_use.name);
                 }

crates/chat-cli/src/cli/chat/tools/internal_command/tool.rs

@@ -19,46 +19,38 @@ use crate::platform::Context;
 
 impl InternalCommand {
     /// Validate that the command exists
-    pub fn validate(&self) -> Result<()> {
-        // Format a command string
-        let cmd_str = if !self.command.starts_with('/') {
-            format!("/{}", self.command)
-        } else {
-            self.command.clone()
-        };
-
-        // Try to parse the command using the Command::parse method
-        match Command::parse(&cmd_str) {
+    pub async fn validate(&self) -> Result<()> {
+        // Parse the command using the existing parse_from_components method
+        match Command::parse_from_components(
+            &self.command,
+            self.subcommand.as_ref(),
+            self.args.as_ref(),
+            self.flags.as_ref(),
+        ) {
             Ok(_) => Ok(()),
             Err(e) => Err(eyre::eyre!("Unknown command: {} - {}", self.command, e)),
         }
     }
 
     /// Check if the command requires user acceptance
     pub fn requires_acceptance(&self) -> bool {
-        // Format a command string
-        let cmd_str = if !self.command.starts_with('/') {
-            format!("/{}", self.command)
-        } else {
-            self.command.clone()
-        };
-
-        // Try to parse the command
-        if let Ok(command) = Command::parse(&cmd_str) {
-            // Get the handler for this command using to_handler()
-            let handler = command.to_handler();
-
-            // Convert args to string slices for the handler
-            let args: Vec<&str> = match &self.subcommand {
-                Some(subcommand) => vec![subcommand.as_str()],
-                None => vec![],
-            };
+        // Parse the command using the existing parse_from_components method
+        match Command::parse_from_components(
+            &self.command,
+            self.subcommand.as_ref(),
+            self.args.as_ref(),
+            self.flags.as_ref(),
+        ) {
+            Ok(command) => {
+                // Get the handler directly from the command
+                // This will automatically use the subcommand's handler when appropriate
+                let handler = command.to_handler();
 
-            return handler.requires_confirmation(&args);
+                // Pass empty args since the handler already knows what command it's for
+                handler.requires_confirmation(&[])
+            },
+            Err(_) => true, // Default to requiring confirmation for unparsable commands
         }
-
-        // For commands not recognized, default to requiring confirmation
-        true
     }
 
     /// Format the command string with subcommand and arguments
@@ -98,22 +90,23 @@ impl InternalCommand {
 
     /// Get a description for the command
     pub fn get_command_description(&self) -> String {
-        // Format a simple command string
-        let cmd_str = if !self.command.starts_with('/') {
-            format!("/{}", self.command)
-        } else {
-            self.command.clone()
-        };
-
-        // Try to parse the command
-        if let Ok(command) = Command::parse(&cmd_str) {
-            // Get the handler for this command using to_handler()
-            let handler = command.to_handler();
-            return handler.description().to_string();
+        // Parse the command using the existing parse_from_components method
+        match Command::parse_from_components(
+            &self.command,
+            self.subcommand.as_ref(),
+            self.args.as_ref(),
+            self.flags.as_ref(),
+        ) {
+            Ok(command) => {
+                // Get the handler for this command using to_handler()
+                let handler = command.to_handler();
+                handler.description().to_string()
+            },
+            Err(_) => {
+                // For commands not recognized, return a generic description
+                "Execute a command in the Q chat system".to_string()
+            },
         }
-
-        // For commands not recognized, return a generic description
-        "Execute a command in the Q chat system".to_string()
     }
 
     /// Queue description for the command execution

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

@@ -136,9 +136,7 @@ impl Tool {
             Tool::UseAws(use_aws) => use_aws.validate(ctx).await,
             Tool::Custom(custom_tool) => custom_tool.validate(ctx).await,
             Tool::GhIssue(gh_issue) => gh_issue.validate(ctx).await,
-            Tool::InternalCommand(internal_command) => internal_command
-                .validate()
-                .map_err(|e| eyre::eyre!("Tool validation failed: {:?}", e)),
+            Tool::InternalCommand(internal_command) => internal_command.validate().await,
             Tool::Thinking(think) => think.validate(ctx).await,
         }
     }
@@ -165,6 +163,7 @@ impl TryFrom<AssistantToolUse> for Tool {
             "internal_command" => {
                 Self::InternalCommand(serde_json::from_value::<InternalCommand>(value.args).map_err(map_err)?)
             },
+            "thinking" => Self::Thinking(serde_json::from_value::<Thinking>(value.args).map_err(map_err)?),
             unknown => {
                 return Err(ToolUseResult {
                     tool_use_id: value.id,
@@ -344,7 +343,7 @@ impl std::fmt::Display for OutputKind {
         match self {
             Self::Text(text) => write!(f, "{}", text),
             Self::Json(json) => write!(f, "{}", json),
-            Self::Images(_) => todo!(),
+            Self::Images(_) => write!(f, ""),
         }
     }
 }