Skip to content

Commit 39fb149

Browse files
ericdalloericdallo
committed
Support deny tool call approval
1 parent 10d14ec commit 39fb149

File tree

8 files changed

+103
-69
lines changed

8 files changed

+103
-69
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
## Unreleased
44

5+
- Support `deny` tool calls via `toolCall approval deny` setting.
6+
57
## 0.43.1
68

79
- Safely rename `default*` -> `select*` in `config/updated`.

‎docs/configuration.md‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,9 @@ For MCP servers configuration, use the `mcpServers` config, example:
7070

7171
### Tool approval / permissions
7272

73-
By default, ECA ask to call any tool, but that's can easily be configureed in many ways via the `toolCall approval` config
73+
By default, ECA ask to call any tool, but that's can easily be configureed in many ways via the `toolCall approval` config.
74+
75+
You can configure the default behavior via `byDefault` and/or configure a tool in `ask`, `allow` or `deny` configs.
7476

7577
Check some examples:
7678

@@ -135,6 +137,22 @@ Check some examples:
135137
}
136138
}
137139
```
140+
141+
=== "Denying a tool"
142+
143+
```javascript
144+
{
145+
"toolCall": {
146+
"approval": {
147+
"byDefault": "allow",
148+
"deny": {
149+
"eca_shell_command": {"argsMatchers" {"command" [".*rm.*",
150+
".*mv.*"]}}
151+
}
152+
}
153+
}
154+
}
155+
```
138156

139157
Also check the `plan` behavior which is safer.
140158

@@ -253,6 +271,7 @@ There are 3 possible ways to configure rules following this order of priority:
253271
byDefault: 'ask' | 'allow';
254272
allow?: {{key: string}: {argsMatchers?: {{[key]: string}: string[]}}},
255273
ask?: {{key: string}: {argsMatchers?: {{[key]: string}: string[]}}},
274+
deny?: {{key: string}: {argsMatchers?: {{[key]: string}: string[]}}},
256275
};
257276
};
258277
mcpTimeoutSeconds?: number;
@@ -300,7 +319,8 @@ There are 3 possible ways to configure rules following this order of priority:
300319
"approval": {
301320
"byDefault": "ask",
302321
"allow": {},
303-
"ask": {}
322+
"ask": {},
323+
"deny": {}
304324
}
305325
},
306326
"mcpTimeoutSeconds" : 60,

‎docs/protocol.md‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -546,12 +546,7 @@ interface ToolCallPrepareContent {
546546
/*
547547
* Argument text of this tool call
548548
*/
549-
argumentsText: string;
550-
551-
/**
552-
* Whether this call requires manual approval from the user.
553-
*/
554-
manualApproval: boolean;
549+
argumentsText: string;
555550

556551
/**
557552
* Summary text to present about this tool call,
@@ -689,7 +684,7 @@ interface ToolCallRejected {
689684
/**
690685
* The reason why this tool call was rejected
691686
*/
692-
reason: 'user';
687+
reason: 'user-choice' | 'user-config';
693688

694689
/**
695690
* Summary text to present about this tool call,

‎src/eca/config.clj‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@
6868
:disabledTools []
6969
:toolCall {:approval {:byDefault "ask"
7070
:allow {}
71-
:ask {}}}
71+
:ask {}
72+
:deny {}}}
7273
:mcpTimeoutSeconds 60
7374
:lspTimeoutSeconds 30
7475
:mcpServers {}
@@ -199,6 +200,8 @@
199200
[:toolCall :approval :allow :ANY :argsMatchers]
200201
[:toolCall :approval :ask]
201202
[:toolCall :approval :ask :ANY :argsMatchers]
203+
[:toolCall :approval :deny]
204+
[:toolCall :approval :deny :ANY :argsMatchers]
202205
[:mcpServers]]}
203206
(deep-merge initialization-config
204207
(when-not pure-config? (config-from-envvar))

‎src/eca/features/chat.clj‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,7 @@
183183
:name name
184184
:origin (tool-name->origin name all-tools)
185185
:arguments-text arguments-text
186-
:id id
187-
:manual-approval (f.tools/manual-approval? all-tools name nil db config)}
186+
:id id}
188187
:summary (f.tools/tool-call-summary all-tools name nil))))
189188
:on-tools-called (fn [tool-calls]
190189
(assert-chat-not-stopped! chat-ctx)
@@ -198,26 +197,27 @@
198197
details (f.tools/tool-call-details-before-invocation name arguments)
199198
summary (f.tools/tool-call-summary all-tools name arguments)
200199
origin (tool-name->origin name all-tools)
201-
manual-approval? (f.tools/manual-approval? all-tools name arguments db config)]
202-
;; Inform UI the tool is about to run and store approval promise
200+
approval (f.tools/approval all-tools name arguments db config)
201+
ask? (= :ask approval)]
202+
;; Inform client the tool is about to run and store approval promise
203203
(send-content! chat-ctx :assistant
204204
(assoc-some
205205
{:type :toolCallRun
206206
:name name
207207
:origin (tool-name->origin name all-tools)
208208
:arguments arguments
209209
:id id
210-
:manual-approval manual-approval?}
210+
:manual-approval ask?}
211211
:details details
212212
:summary summary))
213213
(swap! db* assoc-in [:chats chat-id :tool-calls id :approved?*] approved?*)
214-
(if manual-approval?
214+
(if ask?
215215
(send-content! chat-ctx :system
216216
{:type :progress
217217
:state :running
218218
:text "Waiting for tool call approval"})
219-
;; Otherwise auto approve
220-
(deliver approved?* true))
219+
;; Otherwise decide approval
220+
(deliver approved?* (= :allow approval)))
221221
;; Execute each tool call concurrently
222222
(future
223223
(if @approved?*
@@ -246,19 +246,21 @@
246246
:outputs (:contents result)}
247247
:details details
248248
:summary summary))))
249-
(do
249+
(let [[reject-reason reject-text] (if (= :deny approval)
250+
[:user-config "Tool call denied by user config"]
251+
[:user-choice "Tool call rejected by user choice"])]
250252
(add-to-history! {:role "tool_call" :content tool-call})
251253
(add-to-history! {:role "tool_call_output"
252254
:content (assoc tool-call :output {:error true
253-
:contents [{:text "Tool call rejected by user"
255+
:contents [{:text reject-text
254256
:type :text}]})})
255257
(send-content! chat-ctx :assistant
256258
(assoc-some
257259
{:type :toolCallRejected
258260
:origin origin
259261
:name name
260262
:arguments arguments
261-
:reason :user
263+
:reason reject-reason
262264
:id id}
263265
:details details
264266
:summary summary))))))))]

‎src/eca/features/tools.clj‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -153,37 +153,40 @@
153153
:else
154154
true)))
155155

156-
(defn manual-approval? [all-tools tool-call-name args db config]
157-
(boolean
158-
(let [{:keys [server require-approval-fn]} (first (filter #(= tool-call-name (:name %))
159-
all-tools))
160-
{:keys [allow ask byDefault]} (get-in config [:toolCall :approval])]
161-
(cond
162-
(and require-approval-fn (require-approval-fn args {:db db}))
163-
true
156+
(defn approval
157+
"Return the approval keyword for the specific tool call: ask, allow or deny."
158+
[all-tools tool-call-name args db config]
159+
(let [{:keys [server require-approval-fn]} (first (filter #(= tool-call-name (:name %))
160+
all-tools))
161+
{:keys [allow ask deny byDefault]} (get-in config [:toolCall :approval])]
162+
(cond
163+
(and require-approval-fn (require-approval-fn args {:db db}))
164+
:ask
165+
166+
(some #(approval-matches? % server tool-call-name args) deny)
167+
:deny
164168

165-
(some #(approval-matches? % server tool-call-name args) ask)
166-
true
169+
(some #(approval-matches? % server tool-call-name args) ask)
170+
:ask
167171

168-
(some #(approval-matches? % server tool-call-name args) allow)
169-
false
172+
(some #(approval-matches? % server tool-call-name args) allow)
173+
:allow
170174

171-
(legacy-manual-approval? config tool-call-name)
172-
true
175+
(legacy-manual-approval? config tool-call-name)
176+
:ask
173177

174-
(= "ask" byDefault)
175-
true
178+
(= "ask" byDefault)
179+
:ask
176180

177-
(= "allow" byDefault)
178-
false
181+
(= "allow" byDefault)
182+
:allow
179183

180-
;; TODO suport :deny
181-
;; (= "deny" byDefault)
182-
;; false
184+
(= "deny" byDefault)
185+
:deny
183186

184-
;; A config error, default to manual approve
185-
:else
186-
true))))
187+
;; Probably a config error, default to ask
188+
:else
189+
:ask)))
187190

188191
(defn tool-call-summary [all-tools name args]
189192
(when-let [summary-fn (:summary-fn (first (filter #(= name (:name %))

‎test/eca/features/chat_test.clj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
{:keys [chat-id] :as resp}
1818
(with-redefs [llm-api/complete! (:api-mock mocks)
1919
f.tools/call-tool! (:call-tool-mock mocks)
20-
f.tools/manual-approval? (constantly false)]
20+
f.tools/approval (constantly :allow)]
2121
(f.chat/prompt params (h/db*) (h/messenger) (h/config)))]
2222
(is (match? {:chat-id string? :status :success} resp))
2323
{:chat-id chat-id

‎test/eca/features/tools_test.clj‎

Lines changed: 32 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -71,37 +71,46 @@
7171
{:name "request" :server "web"}
7272
{:name "download" :server "web"}]]
7373
(testing "tool has require-approval-fn which returns true"
74-
(is (true? (f.tools/manual-approval? all-tools "eca_shell" {} {} {}))))
74+
(is (= :ask (f.tools/approval all-tools "eca_shell" {} {} {}))))
7575
(testing "tool has require-approval-fn which returns false we ignore it"
76-
(is (true? (f.tools/manual-approval? all-tools "eca_plan" {} {} {}))))
76+
(is (= :ask (f.tools/approval all-tools "eca_plan" {} {} {}))))
7777
(testing "if legacy-manual-approval present, considers it"
78-
(is (true? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:manualApproval true}}))))
78+
(is (= :ask (f.tools/approval all-tools "request" {} {} {:toolCall {:manualApproval true}}))))
7979
(testing "if approval config is provided"
80-
(testing "when matches allow config, return false"
81-
(is (false? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:approval {:allow {"web__request" {}}}}})))
82-
(is (false? (f.tools/manual-approval? all-tools "eca_read" {} {} {:toolCall {:approval {:allow {"eca_read" {}}}}})))
83-
(is (false? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:approval {:allow {"web" {}}}}}))))
84-
(testing "when matches ask config, return true"
85-
(is (true? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:approval {:ask {"web__request" {}}}}})))
86-
(is (true? (f.tools/manual-approval? all-tools "eca_read" {} {} {:toolCall {:approval {:ask {"eca_read" {}}}}})))
87-
(is (true? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:approval {:ask {"web" {}}}}}))))
80+
(testing "when matches allow config"
81+
(is (= :allow (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:allow {"web__request" {}}}}})))
82+
(is (= :allow (f.tools/approval all-tools "eca_read" {} {} {:toolCall {:approval {:allow {"eca_read" {}}}}})))
83+
(is (= :allow (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:allow {"web" {}}}}}))))
84+
(testing "when matches ask config"
85+
(is (= :ask (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:ask {"web__request" {}}}}})))
86+
(is (= :ask (f.tools/approval all-tools "eca_read" {} {} {:toolCall {:approval {:ask {"eca_read" {}}}}})))
87+
(is (= :ask (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:ask {"web" {}}}}}))))
88+
(testing "when matches deny config"
89+
(is (= :deny (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:deny {"web__request" {}}}}})))
90+
(is (= :deny (f.tools/approval all-tools "eca_read" {} {} {:toolCall {:approval {:deny {"eca_read" {}}}}})))
91+
(is (= :deny (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:deny {"web" {}}}}}))))
8892
(testing "when contains argsMatchers"
8993
(testing "has arg but not matches"
90-
(is (true? (f.tools/manual-approval? all-tools "request" {"url" "http://bla.com"} {}
91-
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}}))))
92-
(testing "has arg and matches"
93-
(is (false? (f.tools/manual-approval? all-tools "request" {"url" "http://foo.com"} {}
94-
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}})))
95-
(is (false? (f.tools/manual-approval? all-tools "request" {"url" "foobar"} {}
96-
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" ["foo.*"]}}}}}}))))
94+
(is (= :ask (f.tools/approval all-tools "request" {"url" "http://bla.com"} {}
95+
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}}))))
96+
(testing "has arg and matches for allow"
97+
(is (= :allow (f.tools/approval all-tools "request" {"url" "http://foo.com"} {}
98+
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}})))
99+
(is (= :allow (f.tools/approval all-tools "request" {"url" "foobar"} {}
100+
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" ["foo.*"]}}}}}}))))
101+
(testing "has arg and matches for deny"
102+
(is (= :deny (f.tools/approval all-tools "request" {"url" "http://foo.com"} {}
103+
{:toolCall {:approval {:deny {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}})))
104+
(is (= :deny (f.tools/approval all-tools "request" {"url" "foobar"} {}
105+
{:toolCall {:approval {:deny {"web__request" {:argsMatchers {"url" ["foo.*"]}}}}}}))))
97106
(testing "has not that arg"
98-
(is (true? (f.tools/manual-approval? all-tools "request" {"crazy-url" "http://foo.com"} {}
99-
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}}))))))
107+
(is (= :ask (f.tools/approval all-tools "request" {"crazy-url" "http://foo.com"} {}
108+
{:toolCall {:approval {:allow {"web__request" {:argsMatchers {"url" [".*foo.*"]}}}}}}))))))
100109
(testing "if no approval config matches"
101110
(testing "checks byDefault"
102111
(testing "when 'ask', return true"
103-
(is (true? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:approval {:byDefault "ask"}}}))))
112+
(is (= :ask (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:byDefault "ask"}}}))))
104113
(testing "when 'allow', return false"
105-
(is (false? (f.tools/manual-approval? all-tools "request" {} {} {:toolCall {:approval {:byDefault "allow"}}})))))
114+
(is (= :allow (f.tools/approval all-tools "request" {} {} {:toolCall {:approval {:byDefault "allow"}}})))))
106115
(testing "fallback to manual approval"
107-
(is (true? (f.tools/manual-approval? all-tools "request" {} {} {})))))))
116+
(is (= :ask (f.tools/approval all-tools "request" {} {} {})))))))

0 commit comments

Comments
 (0)