From f8cf935f9121844186c9532f6d560e6e71fe663e Mon Sep 17 00:00:00 2001 From: Eric Dallo Date: Sun, 2 Nov 2025 19:48:14 -0300 Subject: [PATCH] Fix: only clear chat on successful compact; reset compacting flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clear/replace chat history only when /compact completes successfully to prevent accidental data loss when the compact operation fails (e.g., due to size limits). Always reset :compacting? on chat finish, and gate on-finished-side-effect behind a success? flag. Adds unit tests. Fixes #142 🤖 Generated with [eca](https://eca.dev) Co-Authored-By: eca --- src/eca/features/chat.clj | 49 ++++++++++++++++++--------------- test/eca/features/chat_test.clj | 33 ++++++++++++++++++++-- 2 files changed, 58 insertions(+), 24 deletions(-) diff --git a/src/eca/features/chat.clj b/src/eca/features/chat.clj index f618d0f5..137e9da2 100644 --- a/src/eca/features/chat.clj +++ b/src/eca/features/chat.clj @@ -53,27 +53,32 @@ :output output :error error}))) -(defn finish-chat-prompt! [status {:keys [message chat-id db* metrics config on-finished-side-effect] :as chat-ctx}] - (swap! db* assoc-in [:chats chat-id :status] status) - (f.hooks/trigger-if-matches! :postRequest - {:chat-id chat-id - :prompt message} - {:on-before-action (partial notify-before-hook-action! chat-ctx) - :on-after-action (partial notify-after-hook-action! chat-ctx)} - @db* - config) - (send-content! chat-ctx :system - {:type :progress - :state :finished}) - (when-not (get-in @db* [:chats chat-id :created-at]) - (swap! db* assoc-in [:chats chat-id :created-at] (System/currentTimeMillis))) - (when on-finished-side-effect - (on-finished-side-effect)) - (db/update-workspaces-cache! @db* metrics)) +(defn finish-chat-prompt! + ([status chat-ctx] + (finish-chat-prompt! status chat-ctx true)) + ([status {:keys [message chat-id db* metrics config on-finished-side-effect] :as chat-ctx} success?] + (swap! db* assoc-in [:chats chat-id :status] status) + (f.hooks/trigger-if-matches! :postRequest + {:chat-id chat-id + :prompt message} + {:on-before-action (partial notify-before-hook-action! chat-ctx) + :on-after-action (partial notify-after-hook-action! chat-ctx)} + @db* + config) + (send-content! chat-ctx :system + {:type :progress + :state :finished}) + (when-not (get-in @db* [:chats chat-id :created-at]) + (swap! db* assoc-in [:chats chat-id :created-at] (System/currentTimeMillis))) + ;; Always end any pending compact attempt + (swap! db* assoc-in [:chats chat-id :compacting?] false) + (when (and success? on-finished-side-effect) + (on-finished-side-effect)) + (db/update-workspaces-cache! @db* metrics))) (defn ^:private assert-chat-not-stopped! [{:keys [chat-id db*] :as chat-ctx}] (when (identical? :stopping (get-in @db* [:chats chat-id :status])) - (finish-chat-prompt! :idle chat-ctx) + (finish-chat-prompt! :idle chat-ctx false) (logger/info logger-tag "Chat prompt stopped:" chat-id) (throw (ex-info "Chat prompt stopped" {:silent? true :chat-id chat-id})))) @@ -620,7 +625,7 @@ {:type :text :text (str "API limit reached. Tokens: " (json/generate-string (:tokens msg)))}) - (finish-chat-prompt! :idle chat-ctx)) + (finish-chat-prompt! :idle chat-ctx false)) :finish (do (add-to-history! {:role "assistant" :content [{:type :text :text @received-msgs*}]}) (finish-chat-prompt! :idle chat-ctx)))) @@ -830,7 +835,7 @@ :text "Tell ECA what to do differently for the rejected tool(s)"}) (add-to-history! {:role "user" :content [{:type :text :text "I rejected one or more tool calls with the following reason"}]}))) - (finish-chat-prompt! :idle chat-ctx) + (finish-chat-prompt! :idle chat-ctx false) nil) {:new-messages (get-in @db* [:chats chat-id :messages])}))) :on-reason (fn [{:keys [status id text external-id]}] @@ -861,7 +866,7 @@ (send-content! chat-ctx :system {:type :text :text (or message (str "Error: " (ex-message exception)))}) - (finish-chat-prompt! :idle chat-ctx))})))) + (finish-chat-prompt! :idle chat-ctx false))})))) (defn ^:private send-mcp-prompt! [{:keys [prompt args]} @@ -1084,7 +1089,7 @@ (transition-tool-call! db* chat-ctx tool-call-id :stop-requested {:reason {:code :user-prompt-stop :text "Tool call rejected because of user prompt stop"}})) - (finish-chat-prompt! :stopping chat-ctx)))) + (finish-chat-prompt! :stopping chat-ctx false)))) (defn delete-chat [{:keys [chat-id]} db* metrics] diff --git a/test/eca/features/chat_test.clj b/test/eca/features/chat_test.clj index 07359946..4a5f64d7 100644 --- a/test/eca/features/chat_test.clj +++ b/test/eca/features/chat_test.clj @@ -16,7 +16,7 @@ (defn ^:private prompt! [params mocks] (let [{:keys [chat-id] :as resp} (with-redefs [llm-api/sync-or-async-prompt! (:api-mock mocks) - llm-api/sync-prompt! (constantly nil) + llm-api/sync-prompt! (constantly nil) f.tools/call-tool! (:call-tool-mock mocks) f.tools/approval (constantly :allow)] (h/config! {:env "test"}) @@ -321,7 +321,7 @@ :messages [{:role "user" :content [{:type :text :text "Run 3 read-only tool calls simultaneously."}]} {:role "assistant" :content [{:type :text :text "Ok, working on it"}]} {:role "tool_call" :content {:id "call-3" :name "ro_tool_3" :arguments {}}} - {:role "tool_call_output" :content {:id "call-3" :name "ro_tool_3" :arguments {} + {:role "tool_call_output" :content {:id "call-3" :name "ro_tool_3" :arguments {} :output {:error false :contents [{:type :text, :text "RO tool call 3 result"}]}}} {:role "tool_call" :content {:id "call-2" :name "ro_tool_2" :arguments {}}} @@ -515,3 +515,32 @@ :prompt "prompt" :args ["arg1" "arg2"]} (#'f.chat/message->decision "/server:prompt arg1 arg2"))))) + +(deftest finish-chat-prompt-success-flag-test + (testing "Does not run side effect on failure and resets compacting?" + (let [db* (h/db*) + chat-id "chat-fail-1" + side-effect-called* (atom false) + chat-ctx {:chat-id chat-id + :db* db* + :metrics (h/metrics) + :config (h/config) + :messenger (h/messenger) + :on-finished-side-effect #(reset! side-effect-called* true)}] + (swap! db* assoc-in [:chats chat-id :compacting?] true) + (f.chat/finish-chat-prompt! :idle chat-ctx false) + (is (= false @side-effect-called*) "Side effect should not run on failure") + (is (false? (get-in @db* [:chats chat-id :compacting?])) "compacting? should be reset to false"))) + + (testing "Runs side effect on success" + (let [db* (h/db*) + chat-id "chat-success-1" + side-effect-called* (atom false) + chat-ctx {:chat-id chat-id + :db* db* + :metrics (h/metrics) + :config (h/config) + :messenger (h/messenger) + :on-finished-side-effect #(reset! side-effect-called* true)}] + (f.chat/finish-chat-prompt! :idle chat-ctx true) + (is (= true @side-effect-called*) "Side effect should run on success"))))