diff --git a/CHANGELOG.md b/CHANGELOG.md index f6ed690b..2363996b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Optimize large shell output by writing to cache file instead of inline when exceeding threshold (default: 1000 chars). Configurable via `toolCall.shellCommand.outputFileThreshold`. + ## 0.93.2 - Fix `/compact` removing chat history when prompt is stopped or some error happens. #142 diff --git a/src/eca/config.clj b/src/eca/config.clj index d6516e24..d00df369 100644 --- a/src/eca/config.clj +++ b/src/eca/config.clj @@ -127,7 +127,8 @@ :ask {} :deny {}} :readFile {:maxLines 2000} - :shellCommand {:summaryMaxLength 25}} + :shellCommand {:summaryMaxLength 25 + :outputFileThreshold 1000}} :mcpTimeoutSeconds 60 :lspTimeoutSeconds 30 :mcpServers {} diff --git a/src/eca/features/tools/filesystem.clj b/src/eca/features/tools/filesystem.clj index e7eaa6ee..a4e955a1 100644 --- a/src/eca/features/tools/filesystem.clj +++ b/src/eca/features/tools/filesystem.clj @@ -311,6 +311,14 @@ {:path source :content source-content}])))) +(defn ^:private read-file-require-approval-fn + "Require-approval-fn for read_file that allows ECA shell output cache files + without requiring approval, since the original shell command was already approved." + [args {:keys [db]}] + (when-let [path (get args "path")] + (and (tools.util/path-outside-workspace? db path) + (not (tools.util/eca-shell-output-cache-file? path))))) + (def definitions {"directory_tree" {:description (tools.util/read-tool-description "directory_tree") @@ -334,7 +342,7 @@ :description "Maximum lines to read (default: {{readFileMaxLines}})"}} :required ["path"]} :handler #'read-file - :require-approval-fn (tools.util/require-approval-when-outside-workspace ["path"]) + :require-approval-fn #'read-file-require-approval-fn :summary-fn #'read-file-summary} "write_file" {:description (tools.util/read-tool-description "write_file") diff --git a/src/eca/features/tools/shell.clj b/src/eca/features/tools/shell.clj index 7fe3f76a..040cae9d 100644 --- a/src/eca/features/tools/shell.clj +++ b/src/eca/features/tools/shell.clj @@ -2,11 +2,15 @@ (:require [babashka.fs :as fs] [babashka.process :as p] + [clojure.java.io :as io] [clojure.string :as string] [eca.config :as config] [eca.features.tools.util :as tools.util] [eca.logger :as logger] - [eca.shared :as shared])) + [eca.shared :as shared]) + (:import + [java.security MessageDigest] + [java.util Base64])) (set! *warn-on-reflection* true) @@ -49,7 +53,62 @@ :continue true} input (assoc :in input))))) -(defn ^:private shell-command [arguments {:keys [db tool-call-id call-state-fn state-transition-fn]}] +(defn ^:private workspaces-hash + "Return an 8-char base64 (URL-safe, no padding) key for the given workspace set." + [workspaces] + (let [paths (->> workspaces + (map #(str (fs/absolutize (fs/file (shared/uri->filename (:uri %)))))) + (distinct) + (sort)) + joined (string/join ":" paths) + md (MessageDigest/getInstance "SHA-256") + digest (.digest (doto md (.update (.getBytes ^String joined "UTF-8")))) + encoder (-> (Base64/getUrlEncoder) + (.withoutPadding)) + key (.encodeToString encoder digest)] + (subs key 0 (min 8 (count key))))) + +(defn ^:private shell-output-cache-dir + "Get the directory path for storing shell output files. + Returns: ~/.cache/eca/{workspace-hash}/shell-output/" + [workspaces] + (let [ws-hash (workspaces-hash workspaces)] + (io/file (tools.util/eca-cache-base-dir) ws-hash "shell-output"))) + +(defn ^:private write-output-to-file + "Write shell command output to a file in the cache directory. + Returns the file path on success, or nil on failure." + [output chat-id tool-call-id workspaces] + (try + (let [dir (shell-output-cache-dir workspaces) + filename (str chat-id "-" tool-call-id ".txt") + file-path (io/file dir filename)] + (io/make-parents file-path) + (spit file-path output) + (str file-path)) + (catch Exception e + (logger/warn logger-tag "Failed to write output to file" + {:chat-id chat-id + :tool-call-id tool-call-id + :error (.getMessage e)}) + nil))) + +(defn ^:private format-file-based-response + ([file-path output exit-code file-size] + (format-file-based-response file-path output exit-code file-size 20)) + ([file-path output exit-code file-size tail] + (let [lines (string/split-lines output) + tail-lines (take-last tail lines) + tail-str (string/join "\n" tail-lines) + formatted-size (format "%,d" file-size)] + (str "Shell command output saved to file due to size, you should inspect the file if needed.\n\n" + "File: " file-path "\n" + "Size: " formatted-size " characters\n" + "Exit code: " exit-code "\n\n" + (format "Last %s lines:\n" (count tail-lines)) + tail-str)))) + +(defn ^:private shell-command [arguments {:keys [db tool-call-id chat-id config call-state-fn state-transition-fn]}] (let [command-args (get arguments "command") user-work-dir (get arguments "working_directory") timeout (min (or (get arguments "timeout") default-timeout) max-timeout)] @@ -77,7 +136,7 @@ (tools.util/tool-call-destroy-resource! "eca__shell_command" :process proc) (state-transition-fn :resources-destroyed {:resources [:process]}) {:exit 1 :err msg})))) - {:exit 1 :err "Tool call is :stopping, so shell rpocess not spawned"}) + {:exit 1 :err "Tool call is :stopping, so shell process not spawned"}) (catch Exception e ;; Process did not start, or had an Exception (other than InterruptedException) during execution. (let [msg (or (.getMessage e) "Caught an Exception during execution of the shell tool")] @@ -93,22 +152,109 @@ (state-transition-fn :resources-destroyed {:resources (keys resources)})))))) err (some-> (:err result) string/trim) out (some-> (:out result) string/trim)] - (if (= result ::timeout) + (cond + (= result ::timeout) (do (logger/debug logger-tag "Command timed out after " timeout " ms") (tools.util/single-text-content (str "Command timed out after " timeout " ms") true)) - (do + + (zero? (:exit result)) + (let [output (or out "") + output-size (count output) + threshold (get-in config [:toolCall :shellCommand :outputFileThreshold] 1000)] + (logger/debug logger-tag "Command executed:" result) + (if (> output-size threshold) + ;; Large output: write to file and return summary + (if (and chat-id tool-call-id) + (if-let [file-path (write-output-to-file output chat-id tool-call-id (:workspace-folders db))] + (let [response (tools.util/single-text-content + (format-file-based-response file-path output (:exit result) output-size))] + (logger/debug logger-tag "Returning file-based response") + response) + ;; Fallback: if file write failed, inline the output with a warning + (do + (logger/warn logger-tag "File write failed, falling back to inline output") + (tools.util/single-text-content + (str "Warning: Failed to write output to file, showing inline instead.\n\n" + output)))) + ;; chat-id or tool-call-id is nil, inline the output + (do + (logger/warn logger-tag "chat-id or tool-call-id is nil, inlining large output" + {:chat-id chat-id :tool-call-id tool-call-id}) + (tools.util/single-text-content output))) + ;; Small output: inline with structured response + {:error false + :contents (remove nil? + (concat [{:type :text + :text (str "Exit code: " (:exit result))}] + (when-not (string/blank? err) + [{:type :text + :text (str "Stderr:\n" err)}]) + (when-not (string/blank? out) + [{:type :text + :text (str "Stdout:\n" out)}])))})) + + :else + (let [output (str (when-not (string/blank? out) out) + (when (and (not (string/blank? out)) + (not (string/blank? err))) + "\n") + (when-not (string/blank? err) err)) + output-size (count output) + threshold (get-in config [:toolCall :shellCommand :outputFileThreshold] 1000)] (logger/debug logger-tag "Command executed:" result) - {:error (not (zero? (:exit result))) - :contents (remove nil? - (concat [{:type :text - :text (str "Exit code: " (:exit result))}] - (when-not (string/blank? err) - [{:type :text - :text (str "Stderr:\n" err)}]) - (when-not (string/blank? out) - [{:type :text - :text (str "Stdout:\n" out)}])))})))))) + (if (> output-size threshold) + ;; Large error output: write to file and return summary + (if (and chat-id tool-call-id) + (if-let [file-path (write-output-to-file output chat-id tool-call-id (:workspace-folders db))] + (let [response-text (str "Command failed with exit code " (:exit result) ".\n\n" + "Output saved to file due to size: " file-path "\n" + "Size: " (format "%,d" output-size) " characters\n\n" + (format "Last 20 lines:\n") + (string/join "\n" (take-last 20 (string/split-lines output))))] + (logger/debug logger-tag "Returning file-based error response") + {:error (not (zero? (:exit result))) + :contents [{:type :text + :text response-text}]}) + ;; Fallback: if file write failed, inline the output with a warning + (do + (logger/warn logger-tag "File write failed for error output, falling back to inline") + {:error (not (zero? (:exit result))) + :contents (remove nil? + (concat [{:type :text + :text (str "Warning: Failed to write output to file, showing inline instead.\n\n" + "Exit code: " (:exit result))}] + (when-not (string/blank? err) + [{:type :text + :text (str "Stderr:\n" err)}]) + (when-not (string/blank? out) + [{:type :text + :text (str "Stdout:\n" out)}])))})) + ;; chat-id or tool-call-id is nil, inline the output + (do + (logger/warn logger-tag "chat-id or tool-call-id is nil, inlining large error output" + {:chat-id chat-id :tool-call-id tool-call-id}) + {:error (not (zero? (:exit result))) + :contents (remove nil? + (concat [{:type :text + :text (str "Exit code: " (:exit result))}] + (when-not (string/blank? err) + [{:type :text + :text (str "Stderr:\n" err)}]) + (when-not (string/blank? out) + [{:type :text + :text (str "Stdout:\n" out)}])))})) + ;; Small error output: inline as before + {:error (not (zero? (:exit result))) + :contents (remove nil? + (concat [{:type :text + :text (str "Exit code: " (:exit result))}] + (when-not (string/blank? err) + [{:type :text + :text (str "Stderr:\n" err)}]) + (when-not (string/blank? out) + [{:type :text + :text (str "Stdout:\n" out)}])))}))))))) (defn shell-command-summary [{:keys [args config]}] (let [max-length (get-in config [:toolCall :shellCommand :summaryMaxLength])] diff --git a/src/eca/features/tools/util.clj b/src/eca/features/tools/util.clj index 2eef7708..ef5cc5ef 100644 --- a/src/eca/features/tools/util.clj +++ b/src/eca/features/tools/util.clj @@ -74,6 +74,27 @@ roots (workspace-root-paths db)] (and p (not-any? #(fs/starts-with? p %) roots)))) +(defn eca-cache-base-dir + "Get the base ECA cache directory. + Returns: ~/.cache/eca/ (or $XDG_CACHE_HOME/eca/)" + [] + (let [cache-home (or (System/getenv "XDG_CACHE_HOME") + (io/file (System/getProperty "user.home") ".cache"))] + (io/file cache-home "eca"))) + +(defn eca-shell-output-cache-file? + "Check if a path is within the ECA shell output cache directory. + This is used to auto-allow reading shell output files without additional approval, + since the shell command that generated them was already approved." + [path] + (when path + (try + (let [abs-path (str (fs/canonicalize path)) + cache-base (str (eca-cache-base-dir))] + (and (fs/starts-with? abs-path cache-base) + (string/includes? abs-path "shell-output"))) + (catch Exception _ false)))) + (defn require-approval-when-outside-workspace "Returns a function suitable for tool `:require-approval-fn` that triggers approval when any of the provided `path-keys` in args is outside the diff --git a/test/eca/features/tools/filesystem_test.clj b/test/eca/features/tools/filesystem_test.clj index 6c1a31ac..a3a52d63 100644 --- a/test/eca/features/tools/filesystem_test.clj +++ b/test/eca/features/tools/filesystem_test.clj @@ -122,6 +122,24 @@ {"path" (h/file-path "/foo/qux") "line_offset" 2 "limit" 2} {:db {:workspace-folders [{:uri (h/file-uri "file:///foo/bar/baz") :name "foo"}]}})))))) +(deftest read-file-require-approval-fn-test + (let [require-approval-fn (get-in f.tools.filesystem/definitions ["read_file" :require-approval-fn])] + (testing "Approval required for files outside workspace" + (let [args {"path" (h/file-path "/outside/workspace/file.txt")} + db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]}] + (is (true? (require-approval-fn args {:db db}))))) + (testing "No approval required for files inside workspace" + (let [args {"path" (h/file-path "/project/foo/src/file.txt")} + db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]}] + (is (not (require-approval-fn args {:db db}))))) + (testing "No approval required for ECA shell output cache files" + (let [cache-path (str (System/getProperty "user.home") "/.cache/eca/abc123/shell-output/chat-1-tool-1.txt") + args {"path" cache-path} + db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]}] + ;; ECA shell output cache files are outside workspace but should not require approval + (with-redefs [fs/canonicalize (fn [p] (fs/path p))] + (is (not (require-approval-fn args {:db db})))))))) + (deftest write-file-test (testing "Approval required outside workspace" (let [require-approval-fn (get-in f.tools.filesystem/definitions ["write_file" :require-approval-fn]) diff --git a/test/eca/features/tools/shell_test.clj b/test/eca/features/tools/shell_test.clj index 598c7586..b61bab97 100644 --- a/test/eca/features/tools/shell_test.clj +++ b/test/eca/features/tools/shell_test.clj @@ -3,9 +3,12 @@ [babashka.fs :as fs] [babashka.process :as p] [clojure.test :refer [are deftest is testing]] + [clojure.string :as string] [eca.features.tools.shell :as f.tools.shell] + [eca.features.tools.util :as tools.util] [eca.test-helper :as h] - [matcher-combinators.test :refer [match?]])) + [matcher-combinators.test :refer [match?]] + [clojure.java.io :as io])) (def ^:private call-state-fn (constantly {:status :executing})) (def ^:private state-transition-fn (constantly nil)) @@ -145,3 +148,147 @@ "pwd" "date" "env"))) + +(deftest large-output-test + (testing "Small output (below threshold) is inlined directly" + (let [small-output "This is a small output"] + (is (match? + {:error false + :contents [{:type :text :text "Exit code: 0"} + {:type :text :text (str "Stdout:\n" small-output)}]} + (with-redefs [fs/exists? (constantly true) + p/process (constantly (future {:exit 0 :out small-output}))] + ((get-in f.tools.shell/definitions ["shell_command" :handler]) + {"command" "echo 'hello'"} + {:db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]} + :config {:toolCall {:shellCommand {:outputFileThreshold 2000}}} + :chat-id "test-chat-123" + :tool-call-id "test-tool-456" + :call-state-fn call-state-fn + :state-transition-fn state-transition-fn})))))) + + (testing "Large output (above threshold) with zero exit is written to file" + (let [large-output (apply str (repeat 3000 "x")) + written-files (atom [])] + (is (match? + {:error false + :contents [{:type :text + :text #"Shell command output saved to file due to size"}]} + (with-redefs [fs/exists? (constantly true) + p/process (constantly (future {:exit 0 :out large-output})) + io/make-parents (constantly nil) + clojure.core/spit (fn [path content] + (swap! written-files conj {:path (str path) :content content}))] + ((get-in f.tools.shell/definitions ["shell_command" :handler]) + {"command" "find ."} + {:db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]} + :config {:toolCall {:shellCommand {:outputFileThreshold 2000}}} + :chat-id "test-chat-123" + :tool-call-id "test-tool-456" + :call-state-fn call-state-fn + :state-transition-fn state-transition-fn})))) + (is (= 1 (count @written-files)) "Should write exactly one file") + (is (= large-output (:content (first @written-files))) "File content should match output"))) + + (testing "Large output (above threshold) with non-zero exit is written to file" + (let [large-output (apply str (repeat 3000 "x")) + written-files (atom [])] + (is (match? + {:error true + :contents [{:type :text + :text #"(?s)Command failed.*exit code 1.*Output saved.*Last 20 lines.*"}]} + (with-redefs [fs/exists? (constantly true) + io/make-parents (constantly nil) + clojure.core/spit (fn [path content] + (swap! written-files conj {:path (str path) :content content})) + p/process (constantly (future {:exit 1 :out large-output :err ""}))] + ((get-in f.tools.shell/definitions ["shell_command" :handler]) + {"command" "false"} + {:db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]} + :config {:toolCall {:shellCommand {:outputFileThreshold 2000}}} + :chat-id "test-chat-123" + :tool-call-id "test-tool-456" + :call-state-fn call-state-fn + :state-transition-fn state-transition-fn})))) + (is (= 1 (count @written-files)) "Should write exactly one file") + (is (= large-output (:content (first @written-files))) "File content should match output"))) + + (testing "File write failure falls back to inline output with warning" + (let [large-output (apply str (repeat 3000 "x"))] + (is (match? + {:error false + :contents [{:type :text + :text #"Warning: Failed to write output to file"}]} + (with-redefs [fs/exists? (constantly true) + p/process (constantly (future {:exit 0 :out large-output})) + io/make-parents (constantly nil) + clojure.core/spit (fn [_ _] (throw (Exception. "Write failed")))] + ((get-in f.tools.shell/definitions ["shell_command" :handler]) + {"command" "find ."} + {:db {:workspace-folders [{:uri (h/file-uri "file:///project/foo") :name "foo"}]} + :config {:toolCall {:shellCommand {:outputFileThreshold 2000}}} + :chat-id "test-chat-123" + :tool-call-id "test-tool-456" + :call-state-fn call-state-fn + :state-transition-fn state-transition-fn}))))))) + +(deftest format-file-based-response-test + (testing "Formats response with default tail of 20 lines" + (let [output (string/join "\n" (map #(str "Line " %) (range 1 51))) + file-path "/path/to/output.txt" + response (#'f.tools.shell/format-file-based-response file-path output 0 1234)] + (is (string/includes? response "Shell command output saved to file")) + (is (string/includes? response file-path)) + (is (string/includes? response "1,234 characters")) + (is (string/includes? response "Exit code: 0")) + (is (string/includes? response "Last 20 lines:")) + (is (string/includes? response "Line 50")) + (is (not (string/includes? response "Line 20"))))) + + (testing "Formats response with custom tail line count" + (let [output (string/join "\n" (map #(str "Line " %) (range 1 51))) + file-path "/path/to/output.txt" + response (#'f.tools.shell/format-file-based-response file-path output 0 1234 5)] + (is (string/includes? response "Last 5 lines:")) + (is (string/includes? response "Line 50")) + (is (not (string/includes? response "Line 40")))))) + +(deftest workspaces-hash-test + (testing "Generates consistent 8-char hash for same workspaces" + (let [workspaces [{:uri "file:///path/to/workspace"}] + hash1 (#'f.tools.shell/workspaces-hash workspaces) + hash2 (#'f.tools.shell/workspaces-hash workspaces)] + (is (= hash1 hash2)) + (is (= 8 (count hash1))))) + + (testing "Generates different hashes for different workspaces" + (let [workspaces1 [{:uri "file:///path/to/workspace1"}] + workspaces2 [{:uri "file:///path/to/workspace2"}] + hash1 (#'f.tools.shell/workspaces-hash workspaces1) + hash2 (#'f.tools.shell/workspaces-hash workspaces2)] + (is (not= hash1 hash2))))) + +(deftest shell-output-cache-dir-test + (testing "Returns correct cache directory path" + (let [workspaces [{:uri "file:///path/to/workspace"}] + dir (#'f.tools.shell/shell-output-cache-dir workspaces) + dir-str (str dir) + ;; Normalize to forward slashes for cross-platform test + normalized-str (string/replace dir-str "\\" "/")] + (is (string/includes? normalized-str ".cache/eca")) + (is (string/includes? normalized-str "shell-output"))))) + +(deftest eca-shell-output-cache-file-test + (testing "Returns true for paths in the ECA shell output cache directory" + (let [cache-path (str (System/getProperty "user.home") "/.cache/eca/abc123/shell-output/chat-1-tool-1.txt")] + (with-redefs [fs/canonicalize (fn [p] (fs/path p))] + (is (true? (tools.util/eca-shell-output-cache-file? cache-path)))))) + (testing "Returns false for paths outside ECA cache" + (is (false? (tools.util/eca-shell-output-cache-file? "/some/other/path.txt"))) + (is (false? (tools.util/eca-shell-output-cache-file? "/home/user/project/file.clj")))) + (testing "Returns false for paths in ECA cache but not shell-output" + (let [cache-path (str (System/getProperty "user.home") "/.cache/eca/abc123/other/file.txt")] + (with-redefs [fs/canonicalize (fn [p] (fs/path p))] + (is (false? (tools.util/eca-shell-output-cache-file? cache-path)))))) + (testing "Returns falsy for nil path" + (is (not (tools.util/eca-shell-output-cache-file? nil)))))