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