Require approval for eca_shell_command if running outside workspace folders.

Fixes #86

Author: ericdallo

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Require approval for `eca_shell_command` if running outside workspace folders.
+
 ## 0.36.5
 
 - Fix pricing for models being case insensitive on its name when checking capabilities.

src/eca/features/chat.clj

@@ -177,7 +177,7 @@
                                                :origin (tool-name->origin name all-tools)
                                                :arguments-text arguments-text
                                                :id id
-                                               :manual-approval (f.tools/manual-approval? name config)}
+                                               :manual-approval (f.tools/manual-approval? all-tools name nil db config)}
                                               :summary (f.tools/tool-call-summary all-tools name nil))))
       :on-tools-called (fn [tool-calls]
                          (assert-chat-not-stopped! chat-ctx)
@@ -191,7 +191,7 @@
                                               details (f.tools/tool-call-details-before-invocation name arguments)
                                               summary (f.tools/tool-call-summary all-tools name arguments)
                                               origin (tool-name->origin name all-tools)
-                                              manual-approval? (f.tools/manual-approval? name config)]
+                                              manual-approval? (f.tools/manual-approval? all-tools name arguments db config)]
                                             ;; Inform UI the tool is about to run and store approval promise
                                           (send-content! chat-ctx :assistant
                                                          (assoc-some

src/eca/features/tools.clj

@@ -116,12 +116,15 @@
                                                                         (assoc :type :mcp)
                                                                         (update :tools #(mapv with-tool-status %)))))})))
 
-(defn manual-approval? [name config]
+(defn manual-approval? [all-tools name args db config]
   (boolean
-   (let [manual-approval? (get-in config [:toolCall :manualApproval] nil)]
-     (if (coll? manual-approval?)
-       (some #(= name (str %)) manual-approval?)
-       manual-approval?))))
+   (let [require-approval-fn (:require-approval-fn (first (filter #(= name (:name %))
+                                                                  all-tools)))
+         manual-approval? (get-in config [:toolCall :manualApproval] nil)]
+     (or (when require-approval-fn (require-approval-fn args {:db db}))
+         (if (coll? manual-approval?)
+           (some #(= name (str %)) manual-approval?)
+           manual-approval?)))))
 
 (defn tool-call-summary [all-tools name args]
   (when-let [summary-fn (:summary-fn (first (filter #(= name (:name %))

src/eca/features/tools/filesystem.clj

@@ -6,7 +6,7 @@
    [clojure.string :as string]
    [eca.diff :as diff]
    [eca.features.tools.util :as tools.util]
-   [eca.shared :as shared]))
+   [eca.shared :as shared :refer [multi-str]]))
 
 (set! *warn-on-reflection* true)
 
@@ -39,20 +39,20 @@
                is-last? (= index (dec (count filepaths)))
                current-prefix (if is-last? "└── " "├── ")
                next-prefix (if is-last? "    " "│   ")]
-           
+
            ;; Register file/directory in stats
            (if (fs/directory? absolute)
              (swap! (:dir-count stats) inc)
              (swap! (:file-count stats) inc))
-           
+
            (let [current-line (str prefix current-prefix filename "\n")
                  ;; Only recurse if we haven't reached max depth
-                 subtree-output (if (and (fs/directory? absolute) 
-                                          (< current-depth max-depth))
-                                  (tree-walk absolute 
-                                             (str prefix next-prefix) 
-                                             stats 
-                                             max-depth 
+                 subtree-output (if (and (fs/directory? absolute)
+                                         (< current-depth max-depth))
+                                  (tree-walk absolute
+                                             (str prefix next-prefix)
+                                             stats
+                                             max-depth
                                              (inc current-depth))
                                   "")]
              (recur (inc index)
@@ -297,12 +297,12 @@
     :handler #'write-file
     :summary-fn #'write-file-summary}
    "eca_edit_file"
-   {:description  (str "Replace a specific string or content block in a file with new content. "
-                       "Finds the exact original content and replaces it with new content. "
-                       "Be extra careful to format the original-content exactly correctly, "
-                       "taking extra care with whitespace and newlines. "
-                       "Avoid replacing whole functions, methods, or classes, change only the needed code. "
-                       "In addition to replacing strings, this can also be used to prepend, append, or delete contents from a file.")
+   {:description  (multi-str "You must use your `eca_read_file` tool to get the file’s exact contents before attempting an edit."
+                             "This tool will error if you attempt an edit without reading the file.When crafting the `orginal_content`, you must match the original content from the `eca_read_file` tool output exactly, including all indentation (spaces/tabs) and newlines."
+                             "Never include any part of the line number prefix in the `original_content` or `new_content`.The edit will FAIL if the `original_content` is not unique in the file. To resolve this, you must expand the `new_content` to include more surrounding lines of code or context to make it a unique block."
+                             "ALWAYS prefer making small, targeted edits to existing files. Avoid replacing entire functions or large blocks of code in a single step unless absolutely necessary. You can always call this tool multiple times for multiple edits."
+                             "To delete content, provide the content to be removed as the `original_content` and an empty string as the `new_content`."
+                             "To prepend or append content, the `new_content` must contain both the new content and the original content from `old_string`.")
     :parameters  {:type "object"
                   :properties {"path" {:type "string"
                                        :description "The absolute file path to do the replace."}

src/eca/features/tools/shell.clj

@@ -60,7 +60,7 @@
 
 (def definitions
   {"eca_shell_command"
-   {:description (multi-str "Execute an arbitrary shell command and return the output.
+   {:description (multi-str "Executes an arbitrary shell command ensuring proper handling and security measures.
 1. Command Execution:
   - Always quote file paths that contain spaces with double quotes (e.g., cd \" path with spaces/file.txt \")
   - Examples of proper quoting:
@@ -71,6 +71,15 @@
   - After ensuring proper quoting, execute the command.
   - Capture the output of the command.
   - VERY IMPORTANT: You MUST avoid using search command `grep`. Instead use eca_grep to search. You MUST avoid read tools like `cat`, `head`, `tail`, and `ls`, and use eca_read_file or eca_directory_tree.
+  - It is very helpful if you write a clear, concise description of what this command does in 5-10 words.
+  - When issuing multiple commands, use the ';' or '&&' operator to separate them. DO NOT use newlines (newlines are ok in quoted strings).
+  - Try to maintain your current working directory throughout the session by using absolute paths and avoiding usage of `cd`. You may use `cd` if the User explicitly requests it.
+    <good-example>
+    pytest /foo/bar/tests
+    </good-example>
+    <bad-example>
+    cd /foo/bar && pytest tests
+    </bad-example>
 
 # Committing changes with git
 
@@ -186,4 +195,9 @@ Important:
                                                    :description "The directory to run the command in. Default to the first workspace root."}}
                  :required ["command"]}
     :handler #'shell-command
+    :require-approval-fn (fn [args {:keys [db]}]
+                           (when-let [wd (and args (get args "working_directory"))]
+                             (when-let [wd (and (fs/exists? wd) (str (fs/canonicalize wd)))]
+                               (let [workspace-roots (mapv (comp shared/uri->filename :uri) (:workspace-folders db))]
+                                 (not-any? #(fs/starts-with? wd %) workspace-roots)))))
     :summary-fn #'shell-command-summary}})

test/eca/features/tools/shell_test.clj

@@ -74,3 +74,25 @@
             {:db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]}
              :config {:nativeTools {:shell {:enabled true
                                             :excludeCommands ["ls" "rm"]}}}}))))))
+
+(deftest shell-require-approval-fn-test
+  (let [approval-fn (get-in f.tools.shell/definitions ["eca_shell_command" :require-approval-fn])
+        db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]}]
+    (testing "returns nil when working_directory is not provided"
+      (is (nil? (approval-fn nil {:db db})))
+      (is (nil? (approval-fn {} {:db db}))))
+    (testing "returns nil when working_directory does not exist"
+      (with-redefs [fs/exists? (constantly false)]
+        (is (nil? (approval-fn {"working_directory" (h/file-path "/project/foo/src")} {:db db})))))
+    (testing "returns false when working_directory equals a workspace root"
+      (with-redefs [fs/exists? (constantly true)
+                    fs/canonicalize identity]
+        (is (false? (approval-fn {"working_directory" (h/file-path "/project/foo")} {:db db})))))
+    (testing "returns false when working_directory is a subdirectory of a workspace root"
+      (with-redefs [fs/exists? (constantly true)
+                    fs/canonicalize identity]
+        (is (false? (approval-fn {"working_directory" (h/file-path "/project/foo/src")} {:db db})))))
+    (testing "returns true when working_directory is outside any workspace root"
+      (with-redefs [fs/exists? (constantly true)
+                    fs/canonicalize identity]
+        (is (true? (approval-fn {"working_directory" (h/file-path "/other/place")} {:db db})))))))