Merge pull request #109 from bombaywalla/add-toolreject-params

Add missing parameters to toolCallRejected

Author: ericdallo

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Added missing parameters to `toolCallRejected` where possible.  PR #109
+
 ## 0.49.0
 
 - Add `totalTimeMs` to reason and toolCall content blocks.

src/eca/features/chat.clj

@@ -86,7 +86,8 @@
    - promise init & delivery
    - logging/metrics
 
-   Note: all actions are run in the order specified.
+   Note: all actions are run in the order specified.  So, generally, the :send-* actions should be last.
+   Note: The :status is updated before any actions are run, so the actions have the latest :status.
 
    Note: all choices (i.e. conditionals) have to be made in code and result
    in different events sent to the state machine.
@@ -95,15 +96,15 @@
   {;; Note: transition-tool-call! treats no existing state as :initial state
    [:initial :tool-prepare]
    {:status :preparing
-    :actions [:send-toolCallPrepare :init-decision-reason]}
+    :actions [:init-tool-call-state :send-toolCallPrepare]}
 
    [:preparing :tool-prepare]
    {:status :preparing
     :actions [:send-toolCallPrepare]} ; Multiple prepares allowed
 
    [:preparing :tool-run]
    {:status :check-approval
-    :actions [:init-approval-promise :send-toolCallRun]}
+    :actions [:init-arguments :init-approval-promise :send-toolCallRun]}
    ;; TODO: What happens if the promise is created, but no deref happens since the call is stopped?
 
    [:check-approval :approval-ask]
@@ -222,16 +223,20 @@
                     :summary (:summary event-data)))
 
     :send-toolCallRejected
-    (send-content! chat-ctx :assistant
-                   (assoc-some
-                    {:type :toolCallRejected
-                     :id tool-call-id
-                     :origin (:origin event-data)
-                     :name (:name event-data)
-                     :arguments (:arguments event-data)
-                     :reason (:code (:reason event-data) :user)}
-                    :details (:details event-data)
-                    :summary (:summary event-data)))
+    (let [tool-call-state (get-tool-call-state @db* (:chat-id chat-ctx) tool-call-id)
+          name (:name tool-call-state)
+          origin (:origin tool-call-state)
+          arguments (:arguments tool-call-state)]
+      (send-content! chat-ctx :assistant
+                     (assoc-some
+                      {:type :toolCallRejected
+                       :id tool-call-id
+                       :origin (or (:origin event-data) origin)
+                       :name (or (:name event-data) name)
+                       :arguments (or (:arguments event-data) arguments)
+                       :reason (:code (:reason event-data) :user)}
+                      :details (:details event-data)
+                      :summary (:summary event-data))))
 
     ;; State management actions
     :init-approval-promise
@@ -246,10 +251,21 @@
     (deliver (get-in @db* [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :approved?*])
              true)
 
-    :init-decision-reason
-    (swap! db* assoc-in [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :decision-reason]
-           {:reason {:code :none
-                     :text "No reason"}})
+    :init-tool-call-state
+    (swap! db* update-in [:chats (:chat-id chat-ctx) :tool-calls tool-call-id] assoc
+           ;; :status is initialized by the state transition machinery
+           ;; :approval* is initialized by the :init-approval-promise action
+           ;; :arguments is initialized by the :init-arguments action
+           ;; :start-time is initialized by the :set-start-time action
+           :name (:name event-data)
+           :arguments (:arguments event-data)
+           :origin (:origin event-data)
+           :decision-reason {:code :none
+                             :text "No reason"})
+
+    :init-arguments
+    (swap! db* assoc-in [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :arguments]
+           (:arguments event-data))
 
     :set-decision-reason
     (swap! db* assoc-in [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :decision-reason]
@@ -475,8 +491,8 @@
                                                                     :manual-approval ask?
                                                                     :details details
                                                                     :summary summary}))
-                                          ;; assert: In: :check-approval or :stopped
-                                          (when-not (#{:stopped} (:status (get-tool-call-state @db* chat-id id)))
+                                          ;; assert: In: :check-approval or :stopped or :rejected
+                                          (when-not (#{:stopped :rejected} (:status (get-tool-call-state @db* chat-id id)))
                                             (case approval
                                               :ask (transition-tool-call! db* chat-ctx id :approval-ask
                                                                           {:state :running

test/eca/features/chat_tool_call_state_test.clj

@@ -121,7 +121,7 @@
       (let [result (#'f.chat/transition-tool-call! db* chat-ctx tool-call-id :tool-prepare event-data)]
 
         (is (match? {:status :preparing
-                     :actions [:send-toolCallPrepare :init-decision-reason]}
+                     :actions [:init-tool-call-state :send-toolCallPrepare]}
                     result)
             "Expected return value to show :preparing status and send toolCallPrepare action")
 
@@ -218,7 +218,7 @@
       (let [result (#'f.chat/transition-tool-call! db* chat-ctx tool-call-id :tool-run run-event-data)]
 
         (is (match? {:status :check-approval
-                     :actions [:init-approval-promise :send-toolCallRun]}
+                     :actions [:init-arguments :init-approval-promise :send-toolCallRun]}
                     result)
             "Expected next state to be :check-approval with actions of :init-approval-promise and :send-toolCallRun")
 
@@ -273,7 +273,7 @@
       ;; Step 2: :preparing -> :check-approval
       (let [result (#'f.chat/transition-tool-call! db* chat-ctx tool-call-id :tool-run run-event-data)]
         (is (match? {:status :check-approval
-                     :actions [:init-approval-promise :send-toolCallRun]}
+                     :actions [:init-arguments :init-approval-promise :send-toolCallRun]}
                     result)
             "Expected transition to :check-approval with init promise and send run actions")
 
@@ -517,6 +517,7 @@
                (#'f.chat/transition-tool-call! db* chat-ctx "tool-rejected" :stop-requested))
               "Expected exception as already rejected tool calls cannot be stopped"))))))
 
+;; TODO: This test and the previous test seem to be testing similar things.  Clean up.
 (deftest transition-tool-call-stop-transitions-test
   (testing "Stop transitions from various states"
     (h/reset-components!)
@@ -600,7 +601,14 @@
             "Expected system message to contain 'stopped' text")
 
         (is (< 0 (count tool-reject-messages))
-            "Expected at least one toolCallRejected notification to be sent for active tool calls")))))
+            "Expected at least one toolCallRejected notification to be sent for active tool calls")
+
+        (is (every? (comp :id :content) tool-reject-messages)
+            "Expected every toolCallRejected message to have an :id")
+        (is (every? (comp :name :content) tool-reject-messages)
+            "Expected every toolCallRejected message to have a :name")
+        (is (every? (comp :origin :content) tool-reject-messages)
+            "Expected every toolCallRejected message to have an :origin")))))
 
 (deftest test-stop-prompt-message-count
   ;; Test that stop-prompt sends appropriate number of messages
@@ -629,21 +637,38 @@
     (h/reset-components!)
     (let [db* (h/db*)
           chat-id "test-chat"
-          chat-ctx {:chat-id chat-id :request-id "req-123" :messenger (h/messenger)}]
+          tool-call-id "tool-call-1"
+          chat-ctx {:chat-id chat-id :request-id "req-123" :messenger (h/messenger)}
+          approved?* (promise)]
 
       (swap! db* assoc-in [:chats chat-id]
              {:status :running
               :current-request-id "req-123"})
 
-      (#'f.chat/transition-tool-call! db* chat-ctx "tool-1" :tool-prepare
+      (#'f.chat/transition-tool-call! db* chat-ctx tool-call-id :tool-prepare
                                       {:name "list_files" :origin "filesystem" :arguments-text "{}"})
-
+      (#'f.chat/transition-tool-call! db* chat-ctx tool-call-id :tool-run
+                                      {:approved?* approved?*
+                                       :name "list_files"
+                                       :origin "filesystem"
+                                       :arguments {"id" 123 "value" 42}})
       (f.chat/prompt-stop {:chat-id chat-id} db* (h/messenger))
+      (when-not @approved?*
+        (#'f.chat/transition-tool-call! db* chat-ctx tool-call-id :send-reject
+                                        {:approved?* approved?*
+                                         :name "list_files"
+                                         :origin "filesystem"
+                                         :arguments {"id" 123 "value" 42}}))
 
       (let [chat-messages (:chat-content-received (h/messages))
-            message-types (map #(get-in % [:content :type]) chat-messages)]
-        (is (some #{:toolCallRejected} message-types)
-            "Expected toolCallRejected notifications when stopping with active tool calls")))))
+            tool-call-rejected-messages (filter #(= :toolCallRejected (get-in % [:content :type]))
+                                                chat-messages)]
+
+        (is (< 0 (count tool-call-rejected-messages))
+            "Expected toolCallRejected notifications when stopping with active tool calls")
+
+        (is (some #(get-in % [:content :arguments] %) tool-call-rejected-messages)
+            "Expected at least one toolCallRejected message to include non-nil :arguments")))))
 
 (deftest test-stop-prompt-with-non-running-chat
   ;; Test stop-prompt behavior when chat is not running.