Factor ExecTool out of FileSystemTools#1770
Conversation
Summary of ChangesHello @vorburger, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the tool architecture by extracting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the ExecTool out of FileSystemTools, creating a dedicated and more explicit tool for command execution. The new ExecTool is well-isolated, and the built-in filesystem tools are now managed more cleanly. The accompanying documentation updates are excellent, especially the clear security warnings regarding the exec and filesystem tools.
My review includes a few suggestions to improve the robustness of the new ExecTool and maintain class design best practices. Overall, this is a great step towards a more modular and understandable toolset.
| } | ||
|
|
||
| if (!process.waitFor(10, TimeUnit.SECONDS)) { | ||
| process.destroy(); |
There was a problem hiding this comment.
The current implementation calls process.destroy() on timeout, which sends a SIGTERM signal that can be ignored by the process. Since the command has already timed out, it's safer to terminate it forcefully using process.destroyForcibly() to prevent zombie processes.
| process.destroy(); | |
| process.destroyForcibly(); |
| } | ||
| } | ||
|
|
||
| if (!process.waitFor(10, TimeUnit.SECONDS)) { |
There was a problem hiding this comment.
| "grepFile", FunctionTool.create(fileSystemTool, "grepFile"), | ||
| "executeCommand", FunctionTool.create(fileSystemTool, "executeCommand")); | ||
| } | ||
| public class FileSystemTools { |
There was a problem hiding this comment.
The final keyword was removed from the class definition. If this class is not designed for extension, it's a good practice to declare it as final. This prevents subclassing, makes the class's behavior more predictable, and can allow for certain compiler optimizations. If you intend for this class to be subclassed, please consider adding a comment explaining why.
| public class FileSystemTools { | |
| public final class FileSystemTools { |
803a4d0 to
2257400
Compare
2257400 to
6f29888
Compare
Relates to #1644 and #1762 for #1631.
Includes #1768, just to avoid rebase conflicts.