diff --git a/src/eca/features/chat/tool_calls.clj b/src/eca/features/chat/tool_calls.clj index f882fabb9..a1e608c1c 100644 --- a/src/eca/features/chat/tool_calls.clj +++ b/src/eca/features/chat/tool_calls.clj @@ -5,6 +5,7 @@ [eca.features.hooks :as f.hooks] [eca.features.tools :as f.tools] [eca.features.tools.mcp :as f.tools.mcp] + [eca.features.tools.util :as tools.util] [eca.llm-util :as llm-util] [eca.logger :as logger] [eca.shared :as shared :refer [assoc-some]])) @@ -628,8 +629,12 @@ (let [rejected-tool-call-info* (atom nil)] (run! (fn do-tool-call [{:keys [id full-name] :as tool-call}] (let [approved?* (promise) - {:keys [origin name server]} (tool-by-full-name full-name all-tools) + {:keys [origin name server parameters]} (tool-by-full-name full-name all-tools) server-name (:name server) + tool-call (update tool-call :arguments + #(if parameters + (tools.util/omit-optional-empty-string-args parameters %) + %)) decision-plan (decide-tool-call-action tool-call all-tools @db* config agent chat-id {:on-before-hook-action (partial lifecycle/notify-before-hook-action! chat-ctx) diff --git a/src/eca/features/tools.clj b/src/eca/features/tools.clj index 1247defaf..f14567ef8 100644 --- a/src/eca/features/tools.clj +++ b/src/eca/features/tools.clj @@ -189,6 +189,7 @@ all-tools (->> (concat (mapv #(assoc % :origin :native) (native-tools db config)) (mapv #(assoc % :origin :mcp) (f.mcp/all-tools db))) + (mapv #(update % :parameters tools.util/reorder-schema-required-first)) (mapv #(assoc % :full-name (str (-> % :server :name) "__" (:name %)))) (mapv (fn [tool] (update tool :description @@ -222,6 +223,9 @@ db @db* all-tools (all-tools chat-id agent-name db config) tool-meta (some #(when (= full-name (:full-name %)) %) all-tools) + arguments (if-let [parameters (:parameters tool-meta)] + (tools.util/omit-optional-empty-string-args parameters arguments) + arguments) required-args-error (when-let [parameters (:parameters tool-meta)] (tools.util/required-params-error parameters arguments))] (try diff --git a/src/eca/features/tools/agent.clj b/src/eca/features/tools/agent.clj index efb295057..60b7d92de 100644 --- a/src/eca/features/tools/agent.clj +++ b/src/eca/features/tools/agent.clj @@ -150,7 +150,7 @@ ;; Create subagent chat session using deterministic id based on tool-call-id subagent-chat-id (->subagent-chat-id tool-call-id) - user-model (tools.util/normalize-optional-string (get arguments "model")) + user-model (get arguments "model") _ (when user-model (let [available-models (:models db)] (when (and (seq available-models) @@ -167,7 +167,7 @@ ;; Variant validation: reject only when the resolved model has configured ;; variants and the user-specified one isn't among them. Models with no ;; configured variants accept any variant (the LLM API will reject if invalid). - user-variant (tools.util/normalize-optional-string (get arguments "variant")) + user-variant (get arguments "variant") _ (when user-variant (let [valid-variants (model-variant-names config subagent-model)] (when (and (seq valid-variants) @@ -296,8 +296,8 @@ (defmethod tools.util/tool-call-details-before-invocation :spawn_agent [_name arguments _server {:keys [db config chat-id tool-call-id]}] (let [agent-name (get arguments "agent") - user-model (tools.util/normalize-optional-string (get arguments "model")) - user-variant (tools.util/normalize-optional-string (get arguments "variant")) + user-model (get arguments "model") + user-variant (get arguments "variant") subagent (when agent-name (get-agent agent-name config)) parent-model (get-in db [:chats chat-id :model]) diff --git a/src/eca/features/tools/util.clj b/src/eca/features/tools/util.clj index f321e98d3..dd4911f8b 100644 --- a/src/eca/features/tools/util.clj +++ b/src/eca/features/tools/util.clj @@ -109,6 +109,18 @@ (-> (io/resource (str "prompts/tools/" tool-name ".md")) (slurp))) +(defn reorder-schema-required-first + "Returns schema as an ordered map with `type` first, `required` second when + present, and the remaining entries in their existing iteration order." + [{:keys [type required] :as schema}] + (let [first-entries (cond-> [] + type (conj [:type type]) + required (conj [:required required]))] + (if (seq first-entries) + (into (apply array-map (mapcat identity first-entries)) + (remove (fn [[k _]] (contains? #{:type :required} k)) schema)) + schema))) + (defn required-params-error "Given a tool `parameters` JSON schema (object) and an args map, return a single-text-content error when any required parameter is missing. Returns nil @@ -123,6 +135,19 @@ (->> missing (map #(str "`" % "`")) (string/join ", "))) :error))))) +(defn omit-optional-empty-string-args + "Drops optional tool arguments whose value is the empty string. + Required arguments are preserved exactly as provided." + [parameters args] + (let [required (->> (:required parameters) + (map name) + set)] + (into {} + (remove (fn [[k v]] + (and (= "" v) + (not (contains? required (name k)))))) + args))) + (defn ^:private contents->text "Concatenates all text contents from a tool result's :contents into a single string." [contents] @@ -202,7 +227,3 @@ (assoc result :contents [{:type :text :text (str truncated notice)}])) result))))) - -(defn normalize-optional-string - [value] - (some-> value string/trim not-empty)) diff --git a/test/eca/features/tools/util_test.clj b/test/eca/features/tools/util_test.clj index 1244b6568..a37f8b167 100644 --- a/test/eca/features/tools/util_test.clj +++ b/test/eca/features/tools/util_test.clj @@ -148,6 +148,40 @@ config (config-with-truncation 5 10)] (is (= result (tools.util/maybe-truncate-output result config "call-empty")))))) +(deftest reorder-schema-required-first-test + (testing "moves type first and required second when present" + (is (= [[:type "object"] + [:required ["path"]] + [:properties {"path" {:type "string"}}]] + (seq (tools.util/reorder-schema-required-first + {:type "object" + :properties {"path" {:type "string"}} + :required ["path"]})))))) + +(deftest omit-optional-empty-string-args-test + (testing "drops optional empty string arguments" + (is (= {"path" "/tmp/file"} + (tools.util/omit-optional-empty-string-args + {:required ["path"]} + {"path" "/tmp/file" + "pattern" ""})))) + + (testing "preserves required empty string arguments" + (is (= {"path" ""} + (tools.util/omit-optional-empty-string-args + {:required ["path"]} + {"path" ""})))) + + (testing "preserves non-string empty values" + (is (= {"enabled" false + "count" 0 + "tags" []} + (tools.util/omit-optional-empty-string-args + {:required []} + {"enabled" false + "count" 0 + "tags" []}))))) + (deftest path-outside-workspace-allows-tool-call-outputs-dir-test (testing "path inside tool-call-outputs cache dir is not considered outside workspace" (let [db {:workspace-folders [{:uri (h/file-uri "file:///home/user/project") :name "project"}]} diff --git a/test/eca/features/tools_test.clj b/test/eca/features/tools_test.clj index dd136a028..2a020f1c8 100644 --- a/test/eca/features/tools_test.clj +++ b/test/eca/features/tools_test.clj @@ -435,3 +435,67 @@ identity identity nil)))))) + +(deftest call-tool!-omits-optional-empty-string-args-test + (testing "optional empty string args are omitted before native tool invocation" + (let [received-args* (atom nil)] + (is (match? + {:error false + :contents [{:type :text :text "OK"}]} + (with-redefs [f.tools.filesystem/definitions + {"test_optional_empty" + {:description "Test tool optional empty" + :parameters {"type" "object" + :properties {"path" {:type "string"} + "pattern" {:type "string"}} + :required ["path"]} + :handler (fn [args _] + (reset! received-args* args) + {:error false + :contents [{:type :text :text "OK"}]})}}] + (f.tools/call-tool! + "eca__test_optional_empty" + {"path" "/tmp/file" + "pattern" ""} + "chat-3" + "call-4" + "code" + (h/db*) + (h/config) + (h/messenger) + (h/metrics) + identity + identity + nil)))) + (is (= {"path" "/tmp/file"} @received-args*))))) + +(deftest call-tool!-preserves-required-empty-string-args-test + (testing "required empty string args are preserved" + (let [received-args* (atom nil)] + (is (match? + {:error false + :contents [{:type :text :text "OK"}]} + (with-redefs [f.tools.filesystem/definitions + {"test_required_empty" + {:description "Test tool required empty" + :parameters {"type" "object" + :properties {"path" {:type "string"}} + :required ["path"]} + :handler (fn [args _] + (reset! received-args* args) + {:error false + :contents [{:type :text :text "OK"}]})}}] + (f.tools/call-tool! + "eca__test_required_empty" + {"path" ""} + "chat-4" + "call-5" + "code" + (h/db*) + (h/config) + (h/messenger) + (h/metrics) + identity + identity + nil)))) + (is (= {"path" ""} @received-args*))))) diff --git a/test/eca/llm_providers/openai_chat_test.clj b/test/eca/llm_providers/openai_chat_test.clj index 76ab227ef..cb70f6f9f 100644 --- a/test/eca/llm_providers/openai_chat_test.clj +++ b/test/eca/llm_providers/openai_chat_test.clj @@ -2,6 +2,7 @@ (:require [clojure.test :refer [deftest is testing]] [eca.client-test-helpers :refer [with-client-proxied]] + [eca.features.tools.util :as tools.util] [eca.llm-providers.openai-chat :as llm-providers.openai-chat] [matcher-combinators.test :refer [match?]])) @@ -124,6 +125,22 @@ :properties {:location {:type "string"}}} :other-field "ignored"}])))) + (testing "preserves normalized schema order in emitted parameters" + (is (= [[:type "object"] + [:required ["location"]] + [:properties {:location {:type "string"}}]] + (-> (#'llm-providers.openai-chat/->tools + [{:full-name "eca__get_weather" + :description "Get the weather" + :parameters (tools.util/reorder-schema-required-first + {:required ["location"] + :type "object" + :properties {:location {:type "string"}}})}]) + first + :function + :parameters + seq)))) + (testing "Empty tools list" (is (match? []