branch: externals/ellama
commit 4cbcabafb570d7e4fce3b01ef3fdf629f0739226
Author: Sergey Kostyaev <[email protected]>
Commit: Sergey Kostyaev <[email protected]>
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.
---
ellama-tools.el | 45 ++++++++++++++++----
tests/test-ellama.el | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 154 insertions(+), 8 deletions(-)
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/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
