Skip to content

Commit c7b0a2a

Browse files
bombaywallabombaywalla
committed
Handle transition in :waiting-approval with a :stop-requested event.
This is a work-in-progress checkin. Not everything works just yet. In particular, there are concerns that the approval promise will not be derefed if we get a :stop-requested and will cause a memory leak. Also, tests have not been updated yet. I feel that the current tests are too brittle and they need to be changed or removed. Removed the unused :deliver-default-approval action. Changed the name of the :record-metrics action to :log-metrics for consistency and accuracy. Updated the state machine to go to :rejected, rather than :stopped, on a :stop-requested event. Added guards to proceed down the execution path only if not in :stopped. Added actions to store the reason for the tool call decision - both for allows and denys.
1 parent 18f51c9 commit c7b0a2a

File tree

1 file changed

+63
-37
lines changed

1 file changed

+63
-37
lines changed

‎src/eca/features/chat.clj‎

Lines changed: 63 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@
9393
{;; Note: transition-tool-call! treats no existing state as :initial state
9494
[:initial :tool-prepare]
9595
{:status :preparing
96-
:actions [:send-toolCallPrepare]}
96+
:actions [:send-toolCallPrepare :init-decision-reason]}
9797

9898
[:preparing :tool-prepare]
9999
{:status :preparing
@@ -102,26 +102,27 @@
102102
[:preparing :tool-run]
103103
{:status :check-approval
104104
:actions [:init-approval-promise :send-toolCallRun]}
105+
;; TODO: What happens if the promise is created, but no deref happens since the call is stopped?
105106

106107
[:check-approval :config-ask]
107108
{:status :waiting-approval
108109
:actions [:send-progress]}
109110

110111
[:check-approval :config-allow]
111112
{:status :execution-approved
112-
:actions [:deliver-approval-true]}
113+
:actions [:set-decision-reason :deliver-approval-true]}
113114

114115
[:check-approval :config-deny]
115116
{:status :rejected
116-
:actions [:deliver-approval-false]}
117+
:actions [:set-decision-reason :deliver-approval-false]}
117118

118119
[:waiting-approval :user-approve]
119120
{:status :execution-approved
120-
:actions [:deliver-approval-true]}
121+
:actions [:set-decision-reason :deliver-approval-true]}
121122

122123
[:waiting-approval :user-reject]
123124
{:status :rejected
124-
:actions [:deliver-approval-false :log-rejection]}
125+
:actions [:set-decision-reason :deliver-approval-false :log-rejection]}
125126

126127
[:rejected :send-reject]
127128
{:status :rejected
@@ -133,28 +134,28 @@
133134

134135
[:executing :execution-end]
135136
{:status :completed
136-
:actions [:send-toolCalled :record-metrics]}
137+
:actions [:send-toolCalled :log-metrics]}
137138

138139
;; And now all the :stop-requested transitions
139140

140-
;; TODO: In the future, when calls can be interrupted,
141-
;; more states and actions will be required.
141+
;; TODO: In the future, when calls can be interrupted, more states and actions will be required.
142+
;; Therefore, currently, there is no transition from :executing on a :stop-requested event.
142143

143144
[:execution-approved :stop-requested]
144145
{:status :stopped
145146
:actions [:send-toolCallRejected]}
146147

147148
[:waiting-approval :stop-requested]
148-
{:status :stopped
149-
:actions [:deliver-approval-false]}
149+
{:status :rejected
150+
:actions [:set-decision-reason :deliver-approval-false]}
150151

151152
[:check-approval :stop-requested]
152-
{:status :stopped
153-
:actions [:send-toolCallRejected]}
153+
{:status :rejected
154+
:actions [:set-decision-reason :send-toolCallRejected]}
154155

155156
[:preparing :stop-requested]
156157
{:status :stopped
157-
:actions [:send-toolCallRejected]}
158+
:actions [:set-decision-reason :send-toolCallRejected]}
158159

159160
[:initial :stop-requested] ; Nothing sent yet, just mark as stopped
160161
{:status :stopped
@@ -231,16 +232,21 @@
231232
(deliver (get-in @db* [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :approved?*])
232233
true)
233234

234-
:deliver-default-approval
235-
(deliver (get-in @db* [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :approved?*])
236-
(:default-approval event-data))
235+
:init-decision-reason
236+
(swap! db* assoc-in [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :decision-reason]
237+
{:reason {:code :none
238+
:text "No reason"}})
237239

238-
;; Logging/metrics actions
240+
:set-decision-reason
241+
(swap! db* assoc-in [:chats (:chat-id chat-ctx) :tool-calls tool-call-id :decision-reason]
242+
(:reason event-data))
243+
244+
;; Logging actions
239245
:log-rejection
240246
(logger/info logger-tag "Tool call rejected"
241247
{:tool-call-id tool-call-id :reason (:reason event-data)})
242248

243-
:record-metrics
249+
:log-metrics
244250
(logger/debug logger-tag "Tool call completed"
245251
{:tool-call-id tool-call-id :duration (:duration event-data)})
246252

@@ -266,6 +272,9 @@
266272
transition-key [current-status event]
267273
{:keys [status actions]} (get tool-call-state-machine transition-key)]
268274

275+
(logger/debug logger-tag "Tool call transition"
276+
{:current-status current-status :event event :status status})
277+
269278
(when-not status
270279
(let [valid-events (map second (filter #(= current-status (first %))
271280
(keys tool-call-state-machine)))]
@@ -437,28 +446,39 @@
437446
origin (tool-name->origin name all-tools)
438447
approval (f.tools/approval all-tools name arguments db config)
439448
ask? (= :ask approval)]
449+
;; assert: In :preparing or :stopped
440450
;; Inform client the tool is about to run and store approval promise
441-
(transition-tool-call! db* chat-ctx id :tool-run
442-
{:approved?* approved?*
443-
:name name
444-
:origin (tool-name->origin name all-tools)
445-
:arguments arguments
446-
:manual-approval ask?
447-
:details details
448-
:summary summary})
449-
(case approval
450-
:ask (transition-tool-call! db* chat-ctx id :config-ask
451-
{:state :running
452-
:text "Waiting for tool call approval"})
453-
:allow (transition-tool-call! db* chat-ctx id :config-allow)
454-
:deny (transition-tool-call! db* chat-ctx id :config-deny))
451+
(when-not (#{:stopped} (:status (get-tool-call-state @db* chat-id id)))
452+
(transition-tool-call! db* chat-ctx id :tool-run
453+
{:approved?* approved?*
454+
:name name
455+
:origin (tool-name->origin name all-tools)
456+
:arguments arguments
457+
:manual-approval ask?
458+
:details details
459+
:summary summary}))
460+
;; assert: In: :check-approval or :stopped
461+
(when-not (#{:stopped} (:status (get-tool-call-state @db* chat-id id)))
462+
(case approval
463+
:ask (transition-tool-call! db* chat-ctx id :config-ask
464+
{:state :running
465+
:text "Waiting for tool call approval"})
466+
:allow (transition-tool-call! db* chat-ctx id :config-allow
467+
{:reason {:code :user-config-allow
468+
:text "Tool call allowed by user config"}})
469+
:deny (transition-tool-call! db* chat-ctx id :config-deny
470+
{:reason {:code :user-config-deny
471+
:text "Tool call denied by user config"}})
472+
(logger/warn logger-tag "Unknown value of approval in config"
473+
{:approval approval :tool-call-id id})))
455474
;; Execute each tool call concurrently
456475
(future
457476
(if @approved?* ;TODO: Should there be a timeout here? If so, what would be the state transitions?
458-
;; assert: In :execution-approved state
459-
(do
477+
;; assert: In :execution-approved or :stopped
478+
(when-not (#{:stopped} (:status (get-tool-call-state @db* chat-id id)))
460479
(assert-chat-not-stopped! chat-ctx)
461480
(transition-tool-call! db* chat-ctx id :execution-start)
481+
;; assert: In :executing
462482
(let [result (f.tools/call-tool! name arguments @db* config messenger behavior)
463483
details (f.tools/tool-call-details-after-invocation name arguments details result)]
464484
(add-to-history! {:role "tool_call" :content (assoc tool-call
@@ -629,15 +649,19 @@
629649
:db* db*
630650
:request-id request-id
631651
:messenger messenger}]
632-
(transition-tool-call! db* chat-ctx tool-call-id :user-approve)))
652+
(transition-tool-call! db* chat-ctx tool-call-id :user-approve
653+
{:reason {:code :user-choice-allow
654+
:text "Tool call allowed by user choice"}})))
633655

634656
(defn tool-call-reject [{:keys [chat-id tool-call-id request-id]} db* messenger]
635657
(let [chat-ctx {;; What else is needed?
636658
:chat-id chat-id
637659
:db* db*
638660
:request-id request-id
639661
:messenger messenger}]
640-
(transition-tool-call! db* chat-ctx tool-call-id :user-reject)))
662+
(transition-tool-call! db* chat-ctx tool-call-id :user-reject
663+
{:reason {:code :user-choice-deny
664+
:text "Tool call denied by user choice"}})))
641665

642666
(defn query-context
643667
[{:keys [query contexts chat-id]}
@@ -672,7 +696,9 @@
672696

673697
;; Handle each active tool call
674698
(doseq [[tool-call-id _] (get-active-tool-calls @db* chat-id)]
675-
(transition-tool-call! db* chat-ctx tool-call-id :stop-requested))
699+
(transition-tool-call! db* chat-ctx tool-call-id :stop-requested
700+
{:reason {:code :user-prompt-stop
701+
:text "Tool call rejected because of user prompt stop"}}))
676702
(finish-chat-prompt! :stopping chat-ctx))))
677703

678704
(defn delete-chat

0 commit comments

Comments
 (0)