feat: dont require consent for safe readonly bash commands (#728)

Author: brandonskiser

Cargo.lock

@@ -127,6 +127,7 @@ semver = { version = "1.0.16", features = ["serde"] }
 serde = { version = "1.0.216", features = ["derive", "rc"] }
 serde_json = "1.0.115"
 sha2 = "0.10.6"
+shlex = "1.3.0"
 strum = { version = "0.26.3", features = ["derive"] }
 sysinfo = "0.32.0"
 thiserror = "2.0.3"

Cargo.toml

@@ -57,7 +57,7 @@ serde_json.workspace = true
 shell-color.workspace = true
 shell-words = "1.1"
 shellexpand = "3.1.0"
-shlex = "1.3.0"
+shlex.workspace = true
 sysinfo.workspace = true
 time.workspace = true
 tokio.workspace = true

crates/figterm/Cargo.toml

@@ -67,6 +67,7 @@ rustyline = { version = "14.0.0", features = ["derive"] }
 semver.workspace = true
 serde.workspace = true
 serde_json.workspace = true
+shlex.workspace = true
 spinners = "4.1.0"
 sysinfo.workspace = true
 tempfile.workspace = true

crates/q_cli/Cargo.toml

@@ -92,6 +92,13 @@ impl ConversationState {
             warn!(?next_message, "next_message should not exist");
         }
 
+        let input = if input.is_empty() {
+            warn!("input must not be empty when adding new messages");
+            "Empty prompt".to_string()
+        } else {
+            input
+        };
+
         let msg = UserInputMessage {
             content: input,
             user_input_message_context: Some(UserInputMessageContext {
@@ -142,6 +149,9 @@ impl ConversationState {
     ///    are dropped.
     /// 3. The last message is from the assistant. The last message is dropped if it is from the
     ///    user.
+    /// 4. If the last message is from the assistant and it contains tool uses, and a next user
+    ///    message is set without tool results, then the user message will have cancelled tool
+    ///    results.
     pub fn fix_history(&mut self) {
         // Trim the conversation history by finding the second oldest message from the user without
         // tool results - this will be the new oldest message in the history.
@@ -158,7 +168,7 @@ impl ConversationState {
                             matches!(
                                 m.user_input_message_context.as_ref(),
                                 Some(ctx) if ctx.tool_results.as_ref().is_none_or(|v| v.is_empty())
-                            )
+                            ) && !m.content.is_empty()
                         },
                         ChatMessage::AssistantResponseMessage(_) => false,
                     }
@@ -172,7 +182,6 @@ impl ConversationState {
                 None => {
                     debug!("no valid starting user message found in the history, clearing");
                     self.history.clear();
-
                     // Edge case: if the next message contains tool results, then we have to just
                     // abandon them.
                     match &mut self.next_message {

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

@@ -24,12 +24,34 @@ use super::{
 };
 use crate::cli::chat::truncate_safe;
 
+const READONLY_COMMANDS: &[&str] = &["ls", "cat", "echo", "pwd", "which", "head", "tail"];
+
 #[derive(Debug, Clone, Deserialize)]
 pub struct ExecuteBash {
     pub command: String,
 }
 
 impl ExecuteBash {
+    pub fn requires_consent(&self) -> bool {
+        let Some(args) = shlex::split(&self.command) else {
+            return true;
+        };
+
+        const DANGEROUS_PATTERNS: &[&str] = &["|", "<(", "$(", "`", ">", "&&", "||"];
+        if args
+            .iter()
+            .any(|arg| DANGEROUS_PATTERNS.iter().any(|p| arg.contains(p)))
+        {
+            return true;
+        }
+
+        if let Some(cmd) = args.first() {
+            !READONLY_COMMANDS.contains(&cmd.as_str())
+        } else {
+            true
+        }
+    }
+
     pub async fn invoke(&self, mut updates: impl Write) -> Result<InvokeOutput> {
         // We need to maintain a handle on stderr and stdout, but pipe it to the terminal as well
         let mut child = tokio::process::Command::new("bash")
@@ -171,7 +193,6 @@ mod tests {
         // Verifying stderr
         let v = serde_json::json!({
             "command": "echo Hello, world! 1>&2",
-            "interactive": false
         });
         let out = serde_json::from_value::<ExecuteBash>(v)
             .unwrap()
@@ -205,4 +226,45 @@ mod tests {
             panic!("Expected JSON output");
         }
     }
+
+    #[test]
+    fn test_requires_consent_for_readonly_commands() {
+        let cmds = &[
+            // Safe commands
+            ("ls ~", false),
+            ("ls -al ~", false),
+            ("pwd", false),
+            ("echo 'Hello, world!'", false),
+            ("which aws", false),
+            // Potentially dangerous readonly commands
+            ("echo hi > myimportantfile", true),
+            ("ls -al >myimportantfile", true),
+            ("echo hi 2> myimportantfile", true),
+            ("echo hi >> myimportantfile", true),
+            ("echo $(rm myimportantfile)", true),
+            ("echo `rm myimportantfile`", true),
+            ("echo hello && rm myimportantfile", true),
+            ("echo hello&&rm myimportantfile", true),
+            ("ls nonexistantpath || rm myimportantfile", true),
+            ("echo myimportantfile | xargs rm", true),
+            ("echo myimportantfile|args rm", true),
+            ("echo <(rm myimportantfile)", true),
+            ("cat <<< 'some string here' > myimportantfile", true),
+            ("echo '\n#!/usr/bin/env bash\necho hello\n' > myscript.sh", true),
+            ("cat <<EOF > myimportantfile\nhello world\nEOF", true),
+        ];
+        for (cmd, expected) in cmds {
+            let tool = serde_json::from_value::<ExecuteBash>(serde_json::json!({
+                "command": cmd,
+            }))
+            .unwrap();
+            assert_eq!(
+                tool.requires_consent(),
+                *expected,
+                "expected command: `{}` to have requires_consent: `{}`",
+                cmd,
+                expected
+            );
+        }
+    }
 }

crates/q_cli/src/cli/chat/tools/execute_bash.rs

@@ -85,7 +85,7 @@ impl Tool {
         match self {
             Tool::FsRead(_) => false,
             Tool::FsWrite(_) => true,
-            Tool::ExecuteBash(_) => true,
+            Tool::ExecuteBash(execute_bash) => execute_bash.requires_consent(),
             Tool::UseAws(use_aws) => use_aws.requires_consent(),
         }
     }