Skip to content

Conversation

@ultronozm
Copy link
Contributor

Hi, I sat down and lightly polished the tools I had written earlier, so I figured I'd get them here so that others can use and improve upon them. Happy to update the README if the overall shape of the PR is approved.

* llm-tool-collection.el (llm-tool-collection-deftool): Fix checkdoc
errors in documentation string.
Copy link
Owner

@skissue skissue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sincere apologies for the delay; life unfortunately got in the way. Thanks so much for these! Looks good, just need to add the new tools to the README (follow the existing structure there) 🙂.

* llm-tool-collection.el (llm-tool-collection--view-text): New helper
function.
(view-buffer): Add error handling and use the new helper function.
(view-file): New tool for viewing file contents.
(llm-tool-collection--make-edit): New helper function.
(edit-buffer): Improve error handling and use the helper function.
(edit-file, glob, replace-buffer, replace-file, grep, ls)
(buffer-search, list-buffers, bash): New tools.

* README.org: Add table of contents sections for Search and System.
Add CUSTOM_ID properties to all sections to support navigation in Org.
(view-file, edit-file, glob, replace-file, ls, replace-buffer)
(buffer-search, list-buffers, grep, bash): Add documentation
* llm-tool-collection.el (llm-tool-collection-deftool): Add missing
space after period.
(llm-tool-collection--make-edit): Use grave accent quotes for variable
names in error messages.  Fix error message punctuation.
(edit-file): Improve docstring clarity and format.
(list-buffers): Remove redundant period in error message.
(bash): Remove unused error variable in condition-case.
* README.org (llm-tool-collection-register-with-gptel): Simplify so as
to simply call 'gptel-make-tool'.  The latter function has a setter
that manages the internal 'gptel--known-tools' and 'gptel-tools' lists
and correctly handles duplicates.
@ultronozm
Copy link
Contributor Author

Thanks, went ahead and added them to the README (also added custom_id properties, for navigation in Org). Let me know if I missed anything else

Copy link
Owner

@skissue skissue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Not a blocker, but I am curious about your opinion on two points:

  1. I've seen other examples of tools name them in very simple and explicit vocabulary. For example, instead of grep or bash, should we consider naming tools along the lines of search_text or run_command instead?
  2. I'd like to minimize tool duplication and overlap wherever possible. Is there any specific reason to create a new ls tool instead of modifying the existing list_directory tool? Presumably, we can simply add the new optional parameters, since adding optional parameters doesn't really change existing interfaces too much.

@ultronozm
Copy link
Contributor Author

Good points! Let's see.

  1. I've seen other examples of tools name them in very simple and explicit vocabulary. For example, instead of grep or bash, should we consider naming tools along the lines of search_text or run_command instead?

I like the Unix-style names like grep and bash because they're concise, precise and unambiguous. (Claude Code also uses this approach internally.) By comparison, names like search_text and run_command risk ambiguity -- for instance, search_text could mean searching within a buffer, and run_command could mean executing an Elisp command rather than a shell command.

I do see the value in user-friendly plain-English names. We could consider adding an :alias list (e.g., (:alias ["search_text"])), or would that introduce confusion?

  1. I'd like to minimize tool duplication and overlap wherever possible. Is there any specific reason to create a new ls tool instead of modifying the existing list_directory tool? Presumably, we can simply add the new optional parameters, since adding optional parameters doesn't really change existing interfaces too much.

Another key difference is the output format: list-directory lists full paths, whereas ls returns only base names, like the shell command. Base names are more concise and better for quick glances, while full paths may be better for subsequent file operations. Long term, consolidating these into a single tool with the "full paths" feature controlled by a boolean argument seems like the right approach. Today, the low-level Emacs LLM packages stumble on boolean arguments (ahyatt/llm#173, karthink/gptel#714). A workaround would be to use enums. If you'd prefer to consolidate them immediately, we could try that. Otherwise, I think it's reasonable to keep both for now and to consolidate when the noted issues are resolved.

To focus such discussions, it might help to start collecting common tool use scenarios. This would help clarify which tools work best in practice and provide a foundation for testing. Personally, the vast majority of my tool use so far has just been with edit_buffer. More diverse use cases than this are needed to properly evaluate tools like ls and list_directory.

@ultronozm
Copy link
Contributor Author

Hello, just thought I'd check if anything else was missing on my end here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants