feat: add diff tool and support for chrome-devtools #797
Conversation
Summary of ChangesHello @vinci-grape, 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 enhances the agent's capabilities by introducing a sophisticated 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two major features: an edit_file tool for applying code changes in a diff-like format using an LLM, and support for automated GUI testing with chrome-devtools. The changes are well-structured and add significant capabilities. My review focuses on improving robustness and preventing potential runtime errors. Key feedback includes safer handling of tool arguments, more defensive configuration loading, and ensuring proper process cleanup during testing cycles. These improvements will help make the new features more reliable.
| if tool_call['arguments'] is None: | ||
| response_message.tool_calls[idx]['arguments'] = { | ||
| '__error__': | ||
| 'Original arguments were None, replaced by default.' | ||
| } |
There was a problem hiding this comment.
The current implementation for handling None arguments replaces them with a dictionary {'__error__': '...'}. This is likely to cause a TypeError when the tool function is called with an unexpected __error__ keyword argument. A safer approach would be to use an empty dictionary. This would result in a more informative TypeError about missing required arguments if the tool expects any, which is better than an error about an unexpected keyword.
if tool_call['arguments'] is None:
response_message.tool_calls[idx]['arguments'] = {}| self.edit_file_config = getattr(config.tools.file_system, | ||
| 'edit_file_config', None) | ||
| self.client = OpenAI( | ||
| api_key=self.edit_file_config.api_key, | ||
| base_url=self.edit_file_config.base_url) |
There was a problem hiding this comment.
The code directly accesses config.tools.file_system which could raise an AttributeError if config.tools exists but file_system does not. Additionally, if edit_file_config is not found, self.edit_file_config will be None, causing a crash when accessing .api_key. It's better to handle this gracefully with safer access and clear error messages to ensure the configuration is valid when the tool is enabled.
file_system_config = getattr(config.tools, 'file_system', None)
self.edit_file_config = getattr(file_system_config, 'edit_file_config', None) if file_system_config else None
if not self.edit_file_config:
raise ValueError("'edit_file_config' is missing in the configuration for FileSystemTool.")
self.client = OpenAI(
api_key=self.edit_file_config.api_key,
base_url=self.edit_file_config.base_url)| result = subprocess.run(['npm', 'run', 'dev'], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=5, | ||
| stdin=subprocess.DEVNULL) |
There was a problem hiding this comment.
The pkill -f node commands were removed from check_runtime. This is risky because npm run dev often starts a long-running development server. Without a mechanism to terminate it, the process might be orphaned and cause issues in subsequent runs, such as "address already in use" errors or resource leaks. Consider re-introducing a cleanup step to ensure the node process is terminated after the check.
| path: str = None, | ||
| instructions: str = None, | ||
| code_edit: str = None): | ||
| try: |
There was a problem hiding this comment.
The edit_file function signature allows path to be None, but the function body does not handle this case. If path is None, os.path.join(self.output_dir, path) inside the try block will raise a TypeError. Although the tool definition marks path as required, it's good practice to add a defensive check for path at the beginning of the function to prevent this runtime error. For example: if not path: return "Error: 'path' argument is required.".
Change Summary
Related issue number
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.