♻️ refactor(core): implement tool abstraction for tool execution #2467
Conversation
- Replaced individual tool implementations with a `ToolExecutor` for executing tools. - Introduced `BaseTool` abstract class for defining the interface for all tools. - Implemented `ToolFactory` class for creating tool instances based on the tool name. - Implemented `ToolExecutor` class for executing tools using the factory. - Replaced the switch statement in `Cline.ts` with `ToolExecutor` to execute tools. - Added unit tests for the `BaseTool`, `ToolExecutor`, and `ToolFactory` classes. - Created a `README.md` file to document the tool abstraction layer.
|
| return "fetch_instructions" | ||
| } | ||
|
|
||
| public async execute( |
There was a problem hiding this comment.
I'm a bit confused what this PR delivers. But it's possible I am missing the point completely.
Taking this tool as an example, I was expecting the inheritance from a base class to result in simpler code for each tool.
But if I compare this tool to the existing implementation, it's actually longer (72 lines vs 62 lines here: https://github.com/RooVetGit/Roo-Code/blob/main/src/core/tools/fetchInstructionsTool.ts)
My expectation was that some of the boilerplate code like this...
if (block.partial) {
const partialMessage = JSON.stringify({
...sharedMessageProps,
content: undefined,
} satisfies ClineSayTool)
await cline.ask("tool", partialMessage, block.partial).catch(() => {})
return
} else {
if (!task) {
cline.consecutiveMistakeCount++
pushToolResult(await cline.sayAndCreateMissingParamError("fetch_instructions", "task"))
return
}
... which seems to be repeated for every tool, and pretty much the same every time...
could be handled by the base class (I believe this logic handles partial messages and missing parameters, and could be common for every tool).
That would then result in each individual tool having less code, with that code focussed on what's unique and specific to that tool, rather than every tool re-implementing the same generic bits of functionality.
That was my understanding of the motivation for refactoring to a common base class. And from this implementation, we don't seem to be getting that benefit yet...
There was a problem hiding this comment.
good catch! we should absolutely move as much to the base class as possible.
There was a problem hiding this comment.
Initially I didn't want to modify the existing tools so that the new structure could easily be seen, but I wanted to get feedback on the proof of concept. Maybe my plan was not ideal to show the tool files twice for comparison. What I will do now is modify the existing tools files instead.
I apologise for the confusion
|
I really like the effort and what you are doing here! I saw this pull request and got excited to see how it is coming along and was eager to do a review of # add the remote and check out the branch
]$ git remote add bramburn https://github.com/bramburn/Roo-Code.git
]$ git checkout bramburn/refactor/cline.ts/tools/001
# find the commit that this was based on
]$ git merge-base bramburn/refactor/cline.ts/tools/001 origin/main
75ba1db3ca02807999d5c356d19b6236a7e8bb5e
# tag it for reference
]$ git tag bramburn/refactor/cline.ts/tools/001-base 75ba1db3ca02807999d5c356d19b6236a7e8bb5e
# review stats
]$ git diff --stat -w bramburn/refactor/cline.ts/tools/001-base |grep -i execute
.../tools/implementations/ExecuteCommandTool.ts | 66 +++++
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# this is where we have a problem
# /me scratches head: where is the current file being modified?
./src/core/tools/executeCommandTool.ts #<<< it is nowhere to be found I understand that this is probably an initial walk through to convert this from a function based to a class-based implementation, and the hallmark of git is the ability to inspect changes as part of a refactor; however, by copying the tool content to completely new files we introduce a few problems:
In order to prevent these issues, I would greatly appreciate it if you implement these classes in the existing files, even if that means that the file system hierarchy is uncommon for class naming and such. Here is why:
|
|
@KJ7LNW So when is the appropriate time to break out the classes into their own files? RIght after the in-file change and extensive testing / signoff with no PR's merged in-between? |
I think so, that makes it easy to review and gives us a clean break point. The rename operation may not break existing PR's too much: |
| private registerTools(): void { | ||
| // Register all tools | ||
| this.registerTool(new ReadFileTool()) | ||
| this.registerTool(new WriteToFileTool()) | ||
| this.registerTool(new ListFilesTool()) | ||
| this.registerTool(new SearchFilesTool()) | ||
| this.registerTool(new ExecuteCommandTool()) | ||
| this.registerTool(new BrowserActionTool()) | ||
| this.registerTool(new ApplyDiffTool()) | ||
| this.registerTool(new InsertContentTool()) | ||
| this.registerTool(new SearchAndReplaceTool()) | ||
| this.registerTool(new ListCodeDefinitionNamesTool()) | ||
| this.registerTool(new UseMcpToolTool()) | ||
| this.registerTool(new AccessMcpResourceTool()) | ||
| this.registerTool(new AskFollowupQuestionTool()) | ||
| this.registerTool(new SwitchModeTool()) | ||
| this.registerTool(new AttemptCompletionTool()) | ||
| this.registerTool(new NewTaskTool()) | ||
| this.registerTool(new FetchInstructionsTool()) | ||
| } |
There was a problem hiding this comment.
Can we make tool register to this factory by itself (we can do it in base class), and this class look like a registry than a factory
|
thank you all for the feedback, I'll work through them and aim to complete in the next few days |
Thanks for the feedback, I really appreciate it. |
hannesrudolph
moved this from PR [Pre Approval Review]
to PR [Draft/WIP]
in Roo Code Roadmap
|
Hey @bramburn, |
github-project-automation
bot
moved this from PR [Draft/WIP]
to Done
in Roo Code Roadmap
6 participants
ToolExecutorfor executing tools.BaseToolabstract class for defining the interface for all tools.ToolFactoryclass for creating tool instances based on the tool name.ToolExecutorclass for executing tools using the factory.Cline.tswithToolExecutorto execute tools.BaseTool,ToolExecutor, andToolFactoryclasses.README.mdfile to document the tool abstraction layer.Context
Tool Implementation Refactoring Summary
Overview
This refactoring converted all function-based tool implementations to class-based implementations that extend the
BaseToolabstract class. This change provides a more consistent, maintainable, and extensible approach to implementing tools in the codebase.Changes Made
Converted the following tools from function-based to class-based implementations:
ApplyDiffToolInsertContentToolSearchAndReplaceToolUseMcpToolToolAccessMcpResourceToolAskFollowupQuestionToolSwitchModeToolAttemptCompletionToolNewTaskToolFetchInstructionsToolCreated or updated tests for the following tools:
ListFilesTool.test.tsSearchFilesTool.test.tsSwitchModeTool.test.tsAskFollowupQuestionTool.test.tsAdded documentation:
implementations/README.mdto document the class-based implementation approachBenefits
BaseToolabstract class, reducing code duplication.Future Improvements
Testing
All tools have been tested to ensure they function correctly. The tests cover:
Conclusion
This refactoring has significantly improved the structure and maintainability of the tool implementations in the codebase. The class-based approach provides a more consistent and extensible way to implement tools, making it easier to add new tools and maintain existing ones.
TODO:
Implementation
Screenshots
How to Test
Get in Touch