branch: externals/ellama
commit 205190aac333d032243ab63aff235444d726bf57
Merge: 11051563d8 42e26f53ea
Author: Sergey Kostyaev <[email protected]>
Commit: GitHub <[email protected]>
Merge pull request #389 from s-kostyaev/improve-confirmation
Refactor confirmation system architecture
---
NEWS.org | 6 +++
ellama-tools.el | 45 ++++++++++++++++----
ellama.el | 2 +-
tests/test-ellama.el | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 161 insertions(+), 9 deletions(-)
diff --git a/NEWS.org b/NEWS.org
index 76a4bccc1b..92951ac5d2 100644
--- a/NEWS.org
+++ b/NEWS.org
@@ -1,3 +1,9 @@
+* Version 1.12.13
+- Refactor confirmation system architecture. Split confirmation logic into
+ reusable internal functions and improved wrapper factory. The system now
+ accepts explicit tool names and uses them in confirmation prompts for lambda
+ functions. This change improves ellama confirmation and makes it easier to
+ extend.
* Version 1.12.12
- Add native compilation warnings check. This adds a new check-compile-warnings
Make target that allows developers to verify native compilation warnings.
diff --git a/ellama-tools.el b/ellama-tools.el
index 0c049e0f42..4999e2ba25 100644
--- a/ellama-tools.el
+++ b/ellama-tools.el
@@ -139,16 +139,14 @@ Tools from this list will work without user confirmation."
"Contains hash table of allowed functions.
Key is a function name symbol. Value is a boolean t.")
-(defun ellama-tools-confirm (function &rest args)
- "Ask user for confirmation before calling FUNCTION with ARGS.
+(defun ellama-tools--confirm-call (function function-name &rest args)
+ "Ask for confirmation before calling FUNCTION named FUNCTION-NAME.
+ARGS are passed to FUNCTION.
Generates prompt automatically. User can approve once (y), approve
for all future calls (a), forbid (n), or view the details in a
buffer (v) before deciding. Returns the result of FUNCTION if
approved, \"Forbidden by the user\" otherwise."
- (let ((function-name (if (symbolp function)
- (symbol-name function)
- "anonymous-function"))
- (confirmation (gethash function ellama-tools-confirm-allowed nil)))
+ (let ((confirmation (gethash function ellama-tools-confirm-allowed nil)))
(cond
;; If user has approved all calls, just execute the function
((or confirmation
@@ -238,11 +236,43 @@ approved, \"Forbidden by the user\" otherwise."
(funcall cb result-str)
(or result-str "done")))))))))
+(defun ellama-tools-confirm (function &rest args)
+ "Ask user for confirmation before calling FUNCTION with ARGS."
+ (apply #'ellama-tools--confirm-call
+ function
+ (if (symbolp function)
+ (symbol-name function)
+ "anonymous-function")
+ args))
+
+(defun ellama-tools-confirm-with-name (function name &rest args)
+ "Ask user for confirmation before calling FUNCTION with ARGS.
+NAME is fallback label used when FUNCTION has no symbol name."
+ (apply #'ellama-tools--confirm-call
+ function
+ (if (symbolp function)
+ (symbol-name function)
+ (cond
+ ((stringp name) name)
+ ((symbolp name) (symbol-name name))
+ (t "anonymous-function")))
+ args))
+
+(defun ellama-tools--make-confirm-wrapper (func name)
+ "Make confirmation wrapper for FUNC.
+NAME is fallback label used when FUNC has no symbol name."
+ (if (symbolp func)
+ (lambda (&rest args)
+ (apply #'ellama-tools-confirm func args))
+ (lambda (&rest args)
+ (apply #'ellama-tools-confirm-with-name func name args))))
+
(defun ellama-tools-wrap-with-confirm (tool-plist)
"Wrap a tool's function with automatic confirmation.
TOOL-PLIST is a property list in the format expected by `llm-make-tool'.
Returns a new tool definition with the :function wrapped."
(let* ((func (plist-get tool-plist :function))
+ (name (plist-get tool-plist :name))
(args (plist-get tool-plist :args))
(wrapped-args
(mapcar
@@ -254,8 +284,7 @@ Returns a new tool definition with the :function wrapped."
(intern type))))
(plist-put arg :type wrapped-type)))
args))
- (wrapped-func (lambda (&rest args)
- (apply #'ellama-tools-confirm func args))))
+ (wrapped-func (ellama-tools--make-confirm-wrapper func name)))
;; Return a new plist with the wrapped function
(setq tool-plist (plist-put tool-plist :function wrapped-func))
(plist-put tool-plist :args wrapped-args)))
diff --git a/ellama.el b/ellama.el
index 6dc1d1565e..caa0bbc901 100644
--- a/ellama.el
+++ b/ellama.el
@@ -6,7 +6,7 @@
;; URL: http://github.com/s-kostyaev/ellama
;; Keywords: help local tools
;; Package-Requires: ((emacs "28.1") (llm "0.24.0") (plz "0.8") (transient
"0.7") (compat "29.1") (yaml "1.2.3"))
-;; Version: 1.12.12
+;; Version: 1.12.13
;; SPDX-License-Identifier: GPL-3.0-or-later
;; Created: 8th Oct 2023
diff --git a/tests/test-ellama.el b/tests/test-ellama.el
index 1552d0a505..4bc263b9cc 100644
--- a/tests/test-ellama.el
+++ b/tests/test-ellama.el
@@ -1061,6 +1061,42 @@ region, season, or type)! 🍎🍊"))))
(ert-fail (format "Timeout while waiting result for: %s" cmd)))
result))
+(defun ellama-test--named-tool-no-args ()
+ "Return constant string."
+ "zero")
+
+(defun ellama-test--named-tool-one-arg (arg)
+ "Return ARG with prefix."
+ (format "one:%s" arg))
+
+(defun ellama-test--named-tool-two-args (arg1 arg2)
+ "Return ARG1 and ARG2 with prefix."
+ (format "two:%s:%s" arg1 arg2))
+
+(defun ellama-test--make-confirm-wrapper-old (function)
+ "Make wrapper for FUNCTION using old confirm call style."
+ (lambda (&rest args)
+ (apply #'ellama-tools-confirm function args)))
+
+(defun ellama-test--make-confirm-wrapper-new (function name)
+ "Make wrapper for FUNCTION and NAME using wrapper factory."
+ (ellama-tools--make-confirm-wrapper function name))
+
+(defun ellama-test--invoke-confirm-with-yes (wrapper &rest args)
+ "Call WRAPPER with ARGS and auto-answer confirmation with yes.
+Return list with result and prompt."
+ (let ((ellama-tools-confirm-allowed (make-hash-table))
+ (ellama-tools-allow-all nil)
+ (ellama-tools-allowed nil)
+ result
+ prompt)
+ (cl-letf (((symbol-function 'read-char-choice)
+ (lambda (message _choices)
+ (setq prompt message)
+ ?y)))
+ (setq result (apply wrapper args)))
+ (list result prompt)))
+
(ert-deftest test-ellama-shell-command-tool-empty-success-output ()
(should
(string=
@@ -1118,6 +1154,87 @@ region, season, or type)! 🍎🍊"))))
(when (file-exists-p file)
(delete-file file)))))
+(ert-deftest test-ellama-tools-confirm-wrapped-named-no-args-old-and-new ()
+ (ellama-test--ensure-local-ellama-tools)
+ (let* ((old-wrapper (ellama-test--make-confirm-wrapper-old
+ #'ellama-test--named-tool-no-args))
+ (new-wrapper (ellama-test--make-confirm-wrapper-new
+ #'ellama-test--named-tool-no-args
+ "named_tool"))
+ (old-call (ellama-test--invoke-confirm-with-yes old-wrapper))
+ (new-call (ellama-test--invoke-confirm-with-yes new-wrapper)))
+ (should (equal (car old-call) "zero"))
+ (should (equal (car new-call) "zero"))
+ (should
+ (string-match-p
+ "Allow calling ellama-test--named-tool-no-args with arguments: \\?"
+ (cadr old-call)))
+ (should
+ (string-match-p
+ "Allow calling ellama-test--named-tool-no-args with arguments: \\?"
+ (cadr new-call)))))
+
+(ert-deftest test-ellama-tools-confirm-wrapped-named-one-arg-old-and-new ()
+ (ellama-test--ensure-local-ellama-tools)
+ (let* ((old-wrapper (ellama-test--make-confirm-wrapper-old
+ #'ellama-test--named-tool-one-arg))
+ (new-wrapper (ellama-test--make-confirm-wrapper-new
+ #'ellama-test--named-tool-one-arg
+ "named_tool"))
+ (old-call (ellama-test--invoke-confirm-with-yes old-wrapper "A"))
+ (new-call (ellama-test--invoke-confirm-with-yes new-wrapper "A")))
+ (should (equal (car old-call) "one:A"))
+ (should (equal (car new-call) "one:A"))
+ (should
+ (string-match-p
+ "Allow calling ellama-test--named-tool-one-arg with arguments: A\\?"
+ (cadr old-call)))
+ (should
+ (string-match-p
+ "Allow calling ellama-test--named-tool-one-arg with arguments: A\\?"
+ (cadr new-call)))))
+
+(ert-deftest test-ellama-tools-confirm-wrapped-named-two-args-old-and-new ()
+ (ellama-test--ensure-local-ellama-tools)
+ (let* ((old-wrapper (ellama-test--make-confirm-wrapper-old
+ #'ellama-test--named-tool-two-args))
+ (new-wrapper (ellama-test--make-confirm-wrapper-new
+ #'ellama-test--named-tool-two-args
+ "named_tool"))
+ (old-call (ellama-test--invoke-confirm-with-yes old-wrapper "A" "B"))
+ (new-call (ellama-test--invoke-confirm-with-yes new-wrapper "A" "B")))
+ (should (equal (car old-call) "two:A:B"))
+ (should (equal (car new-call) "two:A:B"))
+ (should
+ (string-match-p
+ "Allow calling ellama-test--named-tool-two-args with arguments: A, B\\?"
+ (cadr old-call)))
+ (should
+ (string-match-p
+ "Allow calling ellama-test--named-tool-two-args with arguments: A, B\\?"
+ (cadr new-call)))))
+
+(ert-deftest test-ellama-tools-confirm-prompt-uses-tool-name-for-lambda ()
+ (ellama-test--ensure-local-ellama-tools)
+ (let* ((ellama-tools-confirm-allowed (make-hash-table))
+ (ellama-tools-allow-all nil)
+ (ellama-tools-allowed nil)
+ (tool-plist `(:function ,(lambda (_arg) "ok")
+ :name "mcp_tool"
+ :args ((:name "arg" :type string))))
+ (wrapped (ellama-tools-wrap-with-confirm tool-plist))
+ (wrapped-func (plist-get wrapped :function))
+ seen-prompt)
+ (cl-letf (((symbol-function 'read-char-choice)
+ (lambda (prompt _choices)
+ (setq seen-prompt prompt)
+ ?n)))
+ (funcall wrapped-func "value"))
+ (should
+ (string-match-p
+ "Allow calling mcp_tool with arguments: value\\?"
+ seen-prompt))))
+
(provide 'test-ellama)
;;; test-ellama.el ends here
