From a5b9b02446cc49294ccd79cb3af1d94065b814f6 Mon Sep 17 00:00:00 2001 From: Jakub Zika Date: Thu, 10 Sep 2026 13:52:41 +0200 Subject: [PATCH] Preserve pre-tool hook feedback in model history Successful preToolCall warnings were visible in the UI but absent from model context. Append them to the matching tool result after post hooks so output replacement cannot erase them, without duplicating denials. --- CHANGELOG.md | 2 + docs/config/hooks.md | 4 +- src/eca/features/chat/tool_calls.clj | 22 ++++++-- test/eca/features/chat/tool_calls_test.clj | 65 ++++++++++++++++++++++ 4 files changed, 86 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d6f6bb12..ae934defb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Send successful `preToolCall` hook `additionalContext` to the model with the tool result, including input repair notices and context-only warnings. + ## 0.159.0 - Fix `spawn_agent` details to include the agent's configured `variant`; it was only sent when the LLM passed the `variant` argument explicitly. diff --git a/docs/config/hooks.md b/docs/config/hooks.md index 51f64c519..0c9de0f3c 100644 --- a/docs/config/hooks.md +++ b/docs/config/hooks.md @@ -315,11 +315,13 @@ Fires before a tool is invoked. Use for argument validation, security checks, or - **Honored output**: - `updatedInput` — merged into the tool arguments (across hooks, later keys win). - `approval` — `"allow"` / `"ask"` / `"deny"` override. Approvals merge by precedence `deny > ask > allow`; a hook `allow` never overrides a config `deny`/`ask`. - - `additionalContext` — with `approval: "deny"`, gives the LLM the rejection context so it can adapt. + - `additionalContext`: on exit `0`, nonblank context is appended to the executed tool's result for the model. With `approval: "deny"`, it supplies the rejection reason instead. - `systemMessage`, `suppressOutput`. - `continue: false` + `stopReason` stops the turn (the LLM sees a neutral placeholder in the tool result; `stopReason` reaches only the user). - **Exit 2** — rejects the tool call; the turn continues. stderr becomes the rejection reason sent to the LLM and shown in the hook's output. +Context is appended in hook order after post hooks, so `replacedOutput` cannot erase it. `visible: false` and `suppressOutput` do not prevent model delivery. Calls rejected or stopped before execution receive no extra context. + **Choosing a denial method:** | Method | Effect | LLM gets info? | When to use | diff --git a/src/eca/features/chat/tool_calls.clj b/src/eca/features/chat/tool_calls.clj index d931e45f2..5af5e6bc4 100644 --- a/src/eca/features/chat/tool_calls.clj +++ b/src/eca/features/chat/tool_calls.clj @@ -61,9 +61,9 @@ (update-in messages [idx :content :output :contents] f) messages))))) -(defn ^:private append-post-tool-additional-context! - "Append additionalContext (wrapped as XML) from a postToolCall hook to the - matching tool_call_output message so LLM sees it in the next round." +(defn ^:private append-tool-additional-context! + "Append hook additionalContext (wrapped as XML) to the matching + tool_call_output message so LLM sees it in the next round." [db* chat-id tool-call-id additional-context] (when-not (string/blank? additional-context) (let [entry {:type :text :text (lifecycle/wrap-additional-context additional-context)}] @@ -174,7 +174,7 @@ :value replaced-output}))) ;; If hook provided additionalContext, append as XML to the tool output (when-let [ac (shared/not-blank (get parsed "additionalContext"))] - (append-post-tool-additional-context! + (append-tool-additional-context! (:db* chat-ctx) (:chat-id chat-ctx) tool-call-id @@ -661,6 +661,7 @@ Returns a plan (data) with: - :decision (:ask | :allow | :deny) - :arguments (potentially modified by hooks) + - :additional-contexts (nonblank context from successful hooks, in execution order) - :approval-override (from hooks) - :tool-call-rejected-by-hook? (boolean, explicit hook rejection via exit 2 or approval:deny) - :tool-call-blocked-by-hook? (boolean, hook rejection or current-turn stop prevents execution) @@ -783,6 +784,10 @@ ;; Return the decision plan (cond-> {:decision final-decision :arguments final-arguments + :additional-contexts (into [] (keep (fn [{:keys [parsed exit]}] + (when (zero? exit) + (shared/not-blank (get parsed "additionalContext"))))) + hook-results) :approval-override approval-override :tool-call-rejected-by-hook? tool-call-rejected-by-hook? :tool-call-blocked-by-hook? tool-call-blocked-by-hook? @@ -795,6 +800,7 @@ (defn on-tools-called! [{:keys [db* config chat-id agent messenger metrics] :as chat-ctx} received-msgs* add-to-history! user-messages] (fn [tool-calls] + (logger/with-chat-context chat-id (get-in @db* [:chats chat-id :parent-chat-id]) ;; postToolCall hooks report continue:false through the tool call state, ;; accumulated per-tool by the state machine action :trigger-post-tool-call-hook. (let [all-tools (f.tools/all-tools chat-id agent @db* config) @@ -937,7 +943,11 @@ :reason :user-stop :details details :summary summary}) - (logger/warn logger-tag "Unexpected value of :status in tool call" {:status status}))))))] + (logger/warn logger-tag "Unexpected value of :status in tool call" {:status status}))) + ;; Append after post hooks so replacedOutput cannot erase + ;; pre-hook feedback. Rejected/unstarted calls skip this path. + (doseq [context (:additional-contexts decision-plan)] + (append-tool-additional-context! db* chat-id id context)))))] (transition-tool-call! db* chat-ctx id @@ -1063,4 +1073,4 @@ (continue-fn all-tools user-messages) {:tools all-tools :new-messages (shared/messages-after-last-compact-marker - (get-in @db* [:chats chat-id :messages]))})))))))))))) + (get-in @db* [:chats chat-id :messages]))}))))))))))))) diff --git a/test/eca/features/chat/tool_calls_test.clj b/test/eca/features/chat/tool_calls_test.clj index 5aee8fbc7..81a08b639 100644 --- a/test/eca/features/chat/tool_calls_test.clj +++ b/test/eca/features/chat/tool_calls_test.clj @@ -508,6 +508,71 @@ (:provider-auth result)) "provider-auth must be returned so providers can reuse refreshed auth metadata")))))) +(deftest on-tools-called!-pre-hook-feedback-test + (doseq [{:keys [label pre post expected-texts blocked?]} + [{:label "context-only warnings preserve hook order and ignore failed hooks" + :pre [{:exit 0 :parsed {"additionalContext" "Repair warning" "suppressOutput" true}} + {:exit 1 :parsed {"additionalContext" "Ignored failure"}} + {:exit 0 :parsed {"additionalContext" "Second warning"}} + {:exit 0 :parsed {"additionalContext" " "}}] + :expected-texts ["result" "Repair warning" "Second warning"]} + {:label "post context and replacement coexist with pre feedback" + :pre [{:exit 0 :parsed {"additionalContext" "Repair notice"}}] + :post [{:exit 0 :parsed {"replacedOutput" "replacement" + "additionalContext" "Post context"}}] + :expected-texts ["replacement" "Post context" "Repair notice"]} + {:label "exit 2 ignores stdout context and keeps stderr rejection once" + :pre [{:exit 2 :raw-error "Denied by stderr" + :parsed {"additionalContext" "Ignored denial context"}}] + :blocked? true + :expected-texts ["Denied by stderr"]} + {:label "explicit denial context is not appended twice" + :pre [{:exit 0 :parsed {"approval" "deny" "additionalContext" "Denied by policy"}}] + :blocked? true + :expected-texts ["Denied by policy"]}]] + (testing label + (h/reset-components!) + (let [db* (h/db*) + chat-id "feedback-chat" + unrelated {:role "tool_call_output" + :content {:id "other" :output {:contents [{:type :text :text "untouched"}]}}} + _ (swap! db* assoc-in [:chats chat-id] + {:status :running :prompt-id "prompt-1" :messages [unrelated] + :tool-calls {"call-1" {:status :preparing}}}) + chat-ctx {:db* db* :chat-id chat-id :config (h/config) + :prompt-id "prompt-1" :provider "openai" :agent :default + :messenger (h/messenger) :metrics (h/metrics)} + all-tools [{:name "test_tool" :full-name "eca__test_tool" + :origin :eca :server {:name "eca"}}] + add-to-history! #(swap! db* update-in [:chats chat-id :messages] conj %)] + (with-redefs [f.tools/all-tools (constantly all-tools) + f.tools/approval (constantly :allow) + f.hooks/trigger-if-matches! + (fn [hook-type _ {:keys [on-after-action]} _ _] + (doseq [result (case hook-type :preToolCall pre :postToolCall post nil)] + (on-after-action (assoc result :name "feedback")))) + f.tools/call-tool! (constantly {:contents [{:type :text :text "result"}]}) + f.tools/tool-call-details-before-invocation (constantly nil) + f.tools/tool-call-details-after-invocation (constantly nil) + f.tools/tool-call-summary (constantly "Test tool") + lifecycle/maybe-renew-auth-token (constantly nil) + lifecycle/send-content! (fn [& _])] + (let [result ((tc/on-tools-called! chat-ctx (atom "") add-to-history! []) + [{:id "call-1" :full-name "eca__test_tool" :arguments {}}]) + messages (get-in @db* [:chats chat-id :messages]) + output (last (filter #(= "tool_call_output" (:role %)) messages)) + texts (mapv :text (get-in output [:content :output :contents]))] + (is (= unrelated (first messages)) "only the matching tool output changes") + (if blocked? + (do + (is (= 1 (count texts)) "rejection has no duplicate context block") + (is (string/includes? (first texts) (first expected-texts)))) + (is (= (into [(first expected-texts)] + (map lifecycle/wrap-additional-context (rest expected-texts))) + texts))) + (is (= messages (:new-messages result)) + "hook feedback is present before the next provider request"))))))) + (deftest on-tools-called!-rejection-returns-fresh-auth-test (testing "rejected subagent path also propagates refreshed auth" ;; Previously rejection branches skipped maybe-renew-auth-token and