Skip to content

Commit 5c18fe0

Browse files
committed
Simplify reasoning callback, fix tool call parse error loop
- Use :delta-reasoning? boolean directly instead of deriving from (some? reasoning-content) - Fix infinite loop when tool JSON is truncated - check valid-tools not completed-tools before continuing conversation
1 parent 71d4102 commit 5c18fe0

File tree

4 files changed

+58
-27
lines changed

4 files changed

+58
-27
lines changed

src/eca/features/chat.clj

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,7 +1130,7 @@
11301130
:arguments-text arguments-text
11311131
:summary (f.tools/tool-call-summary all-tools full-name nil config)})))
11321132
:on-tools-called (on-tools-called! chat-ctx received-msgs* add-to-history!)
1133-
:on-reason (fn [{:keys [status id text external-id reasoning-content]}]
1133+
:on-reason (fn [{:keys [status id text external-id delta-reasoning?]}]
11341134
(assert-chat-not-stopped! chat-ctx)
11351135
(case status
11361136
:started (do (swap! reasonings* assoc-in [id :start-time] (System/currentTimeMillis))
@@ -1142,7 +1142,7 @@
11421142
(add-to-history! {:role "reason"
11431143
:content {:id id
11441144
:external-id external-id
1145-
:delta-reasoning? (some? reasoning-content)
1145+
:delta-reasoning? delta-reasoning?
11461146
:total-time-ms total-time-ms
11471147
:text (get-in @reasonings* [id :text])}})
11481148
(send-content! chat-ctx :assistant {:type :reasonFinished :total-time-ms total-time-ms :id id})))

src/eca/llm_api.clj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@
273273
(on-reason-wrapper {:status :thinking :id reason-id :text reason-text})
274274
(on-reason-wrapper {:status :finished
275275
:id reason-id
276-
:reasoning-content reasoning-content}))
276+
:delta-reasoning? (some? reasoning-content)}))
277277
(on-message-received-wrapper {:type :text :text output-text})
278278
(some-> usage (on-usage-updated))
279279
(if-let [new-result (when (seq tools-to-call)

src/eca/llm_providers/openai_chat.clj

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -227,20 +227,20 @@
227227
(let [all-accumulated (vals @tool-calls*)
228228
completed-tools (->> all-accumulated
229229
(filter #(every? % [:id :full-name :arguments-text]))
230-
(map (fn [{:keys [arguments-text name] :as tool-call}]
230+
(map (fn [{:keys [arguments-text full-name] :as tool-call}]
231231
(try
232232
(assoc tool-call :arguments (json/parse-string arguments-text))
233233
(catch Exception e
234234
(let [error-msg (format "Failed to parse JSON arguments for tool '%s': %s"
235-
name (ex-message e))]
235+
full-name (ex-message e))]
236236
(logger/warn logger-tag error-msg)
237237
(assoc tool-call :arguments {} :parse-error error-msg)))))))
238238
;; Filter out tool calls with parse errors to prevent execution with invalid data
239239
valid-tools (remove :parse-error completed-tools)]
240-
(if (seq completed-tools)
241-
;; We have some completed tools (valid or with errors), so continue the conversation
240+
(if (seq valid-tools)
241+
;; We have valid tools to execute, continue the conversation
242242
(on-tools-called-wrapper valid-tools on-tools-called handle-response)
243-
;; No completed tools at all - let the streaming response provide the actual finish_reason
243+
;; No valid tools (all had parse errors or none accumulated) - don't loop
244244
nil)))
245245

246246
(defn ^:private process-text-think-aware
@@ -314,11 +314,9 @@
314314
[reasoning-state* on-reason]
315315
(let [state @reasoning-state*]
316316
(when (and (:type state) (:id state))
317-
(let [accumulated-reasoning (when (= (:type state) :delta)
318-
(not-empty (:content state)))]
319-
(on-reason (cond-> {:status :finished :id (:id state)}
320-
accumulated-reasoning
321-
(assoc :reasoning-content accumulated-reasoning)))))
317+
(on-reason {:status :finished
318+
:id (:id state)
319+
:delta-reasoning? (= (:type state) :delta)}))
322320
(reset! reasoning-state* {:id nil :type nil :content "" :buffer ""})))
323321

324322
(defn ^:private prune-history
@@ -441,17 +439,11 @@
441439
;; Process reasoning if present (o1 models and compatible providers)
442440
(when-let [reasoning-text (or (:reasoning delta)
443441
(:reasoning_content delta))]
444-
(let [state @reasoning-state*]
445-
(when-not (= (:type state) :delta)
446-
(start-delta-reasoning))
447-
;; Get current state after potentially starting delta reasoning
448-
(let [current-state @reasoning-state*]
449-
;; Accumulate reasoning_content (needed for providers that require echoing it back)
450-
(when (:reasoning_content delta)
451-
(swap! reasoning-state* update :content str reasoning-text))
452-
(on-reason {:status :thinking
453-
:id (:id current-state)
454-
:text reasoning-text}))))
442+
(when-not (= (:type @reasoning-state*) :delta)
443+
(start-delta-reasoning))
444+
(on-reason {:status :thinking
445+
:id (:id @reasoning-state*)
446+
:text reasoning-text}))
455447

456448
;; Check if reasoning just stopped (delta-based)
457449
(let [state @reasoning-state*]

test/eca/llm_providers/openai_chat_test.clj

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -372,22 +372,61 @@
372372
(#'llm-providers.openai-chat/finish-reasoning! reasoning-state* on-reason)
373373
(is (not @on-reason-called?) "on-reason should not be called when :id is nil"))))
374374

375+
(deftest execute-accumulated-tools-with-parse-errors-test
376+
(testing "When all tools have parse errors, should NOT call on-tools-called-wrapper (prevents infinite loop)"
377+
(let [tool-calls* (atom {"rid-0" {:index 0
378+
:id "call-123"
379+
:full-name "eca__write_file"
380+
:arguments-text "{\"path\": \"/tmp/test.txt\", \"content\": \"truncated..."}}) ;; Invalid JSON
381+
wrapper-called? (atom false)
382+
on-tools-called-wrapper (fn [& _] (reset! wrapper-called? true))]
383+
(#'llm-providers.openai-chat/execute-accumulated-tools!
384+
tool-calls*
385+
on-tools-called-wrapper
386+
(fn [_] {:new-messages []})
387+
(fn [& _]))
388+
;; The wrapper should NOT be called because there are no valid tools
389+
(is (not @wrapper-called?)
390+
"on-tools-called-wrapper should NOT be called when all tools have parse errors")))
391+
392+
(testing "When some tools are valid and some have errors, should call wrapper with valid tools only"
393+
(let [tool-calls* (atom {"rid-0" {:index 0
394+
:id "call-123"
395+
:full-name "eca__read_file"
396+
:arguments-text "{\"path\": \"/tmp/test.txt\"}"} ;; Valid JSON
397+
"rid-1" {:index 1
398+
:id "call-456"
399+
:full-name "eca__write_file"
400+
:arguments-text "{\"path\": \"truncated..."}}) ;; Invalid JSON
401+
passed-tools (atom nil)
402+
on-tools-called-wrapper (fn [tools & _] (reset! passed-tools tools))]
403+
(#'llm-providers.openai-chat/execute-accumulated-tools!
404+
tool-calls*
405+
on-tools-called-wrapper
406+
(fn [_] {:new-messages []})
407+
(fn [& _]))
408+
;; Should be called with only the valid tool
409+
(is (= 1 (count @passed-tools)))
410+
(is (= "call-123" (:id (first @passed-tools)))))))
411+
375412
(deftest deepseek-non-stream-reasoning-content-test
376413
(testing "response-body->result captures reasoning_content and normalization uses :text with :delta-reasoning?"
377414
(let [body {:usage {:prompt_tokens 5 :completion_tokens 2}
378415
:choices [{:message {:content "hi"
379416
:reasoning_content "think more"}}]}
380417
result (#'llm-providers.openai-chat/response-body->result body (fn [& _]))
381418
;; Simulate how chat.clj would store this: :text has the content, :delta-reasoning? is the flag
419+
;; In non-streaming, llm_api.clj converts (some? reasoning-content) to :delta-reasoning?
382420
normalized (#'llm-providers.openai-chat/normalize-messages
383421
[{:role "user" :content "Q"}
384422
{:role "reason" :content {:id "r1"
385-
:text (:reasoning-content result)
386-
:delta-reasoning? true}}]
423+
:text (:reason-text result)
424+
:delta-reasoning? (some? (:reasoning-content result))}}]
387425
true
388426
thinking-start-tag
389427
thinking-end-tag)]
390-
(is (= "think more" (:reasoning-content result)))
428+
(is (= "think more" (:reason-text result)))
429+
(is (some? (:reasoning-content result)) "reasoning-content should be present in non-streaming result")
391430
(is (match?
392431
[{:role "user" :content [{:type "text" :text "Q"}]}
393432
{:role "assistant"

0 commit comments

Comments
 (0)