Skip to content

Preserve pre-tool hook feedback in model history #593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zikajk wants to merge 1 commit into from
Closed

Preserve pre-tool hook feedback in model history #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 3 additions & 1 deletion docs/config/hooks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
22 changes: 16 additions & 6 deletions src/eca/features/chat/tool_calls.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)}]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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?
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]))})))))))))))))
65 changes: 65 additions & 0 deletions test/eca/features/chat/tool_calls_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading