feat: create a text editor tool with multiple commands #186
Conversation
| @@ -1,7 +1,7 @@ | |||
| import inspect | |||
| import uuid | |||
| from importlib.metadata import entry_points | |||
| from typing import get_args, get_origin | |||
| from typing import Literal, get_args, get_origin, Any, List, Tuple, Dict, Union | |||
There was a problem hiding this comment.
:nit: we can use list, tuple, and dict directly now
|
interesting, will try this out, could be promising. I did notice that tool calling seemed to work well with parameters in the past, and even with richer CLIs - ie able to use the one "tool" different ways seems to suit it more so than choosing between tools (intuitively that makes sense, as a human, to some limited extent, you don't have the cognitive overload of choosing the tool) |
|
@salman1993 any tips on how to try it out to A/B test? |
|
tried it out with editing just one spot in a yaml file... just FYI: The phrase "Found packages with disallowed licenses" occurs multiple times in the .github/workflows/license-check.yml file, making it necessary to be more
specific in the string replace operation. I'll view more of the file to identify the different locations and modify them accordingly. Let's check the
occurrences separately.
─── .github/workflows/license-check.yml ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
uses: actions/setup-python@v5
with:
python-version: '3.10'
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install tomli requests urllib3
- name: Check licenses
run: |
python .github/workflows/scripts/check_licenses.py \
pyproject.toml || exit_code=$?
if [ "${exit_code:-0}" -ne 0 ]; then
echo "::error::Found packages with disallowed licenses"
exit 1
fi
- name: Check Exchange licenses
run: |
python .github/workflows/scripts/check_licenses.py \
packages/exchange/pyproject.toml || exit_code=$?
if [ "${exit_code:-0}" -ne 0 ]; then
echo "::error::Found packages with disallowed licenses in exchange"
exit 1
fi
─── .github/workflows/license-check.yml ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─── .github/workflows/license-check.yml ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Let's specify the contexts where we succeed the operations, starting with the first one.
─── .github/workflows/license-check.yml ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
echo "::error::Found packages with disallowed licenses"
->
echo "::error::Found packages with disallowed licenses, it was in the goose package."
─── .github/workflows/license-check.yml ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
echo "::error::Found packages with disallowed licenses in exchange"
->
echo "::error::Found packages with disallowed licenses, it was in the goose package."some odd artifacts like:
|
|
so far seems to do a good job - but not pushed too hard today |
| patho = system.to_patho(path) | ||
|
|
||
| if patho.exists() and not system.is_active(path): | ||
| print(f"We are warning the LLM to view before write in write_file, with path={path} and patho={str(patho)}") | ||
| raise ValueError(f"You must view {path} using read_file before you overwrite it") | ||
|
|
||
| self._save_file_history(patho) # Save current content to history |
There was a problem hiding this comment.
:nit: i see a lot of erroneous comments like these in this pr and most of the time they're already self documenting. maybe remove?
| return f"Successfully undid the last edit on {path}" | ||
|
|
||
| @tool | ||
| def text_editor( |
There was a problem hiding this comment.
there's a lot of conflated concerns/responsibilities in a single method. it's a little tough to read and would even harder to test. how about trying out a dispatch pattern?
command_dispatch: dict[str, callable] = {
"create": self._create_file,
"insert": self._insert_string,
"undo_edit": self._undo_edit,
...
}There was a problem hiding this comment.
we should possibly consider creating a text editor abstraction given the complex interactions we have here. maybe these things would play well with the editor integrations people have made plugins for?
There was a problem hiding this comment.
like the suggestion for dispatcher pattern. i opened another PR cause i also batched up the changes for bash and process manager: #191
There was a problem hiding this comment.
@lamchau I think this is the approach that claude took and it seems to work well - a smaller number of tools I think is cognitively more pleasant vs LLM needing to decide which tool to use (also lets us put more smarts in the tool vs relying on LLM always to choose the right way) - so I think this general approach feels right?
There was a problem hiding this comment.
@michaelneale not sure i follow? i think the logic itself is sound just tidying it up
|
changes from this PR are in this one: #191 |
Changes:
Anthropic is using this text editor tool for Computer Use. It might be useful to try this in goose - docs, code. It seems like they prefer to have fewer tools, but each tool has more optional args which are used for specific actions. For example, in goose we have separate tools for read_file, write_file, patch_file but in this case, its a single EditorTool with command as an Enum that routes to the editor action (view, create, insert).
Test:
tool descriptions are correctly passed
