Merge pull request #413 from editor-code-assistant/improve-tool-schema-required-ordering

Surface required tool params first for models

Author: ericdallo

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)

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

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])

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))

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"}]}

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*)))))

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?
          []