Feat/aworld cli text to team #696
Conversation
Summary of ChangesHello @ahgpt, 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 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 a significant new feature for aworld_cli, adding support for built-in agents and a plugin system. This is a great enhancement for extensibility. The changes include updates to agent loading logic, a new Aworld agent, a terminal server for executing commands, and documentation updates.
My review focuses on several areas. I've identified a couple of critical runtime errors in the new terminal_server.py and workspace.py files that need to be addressed. There's also a logic bug in the agent loading mechanism in local.py that could lead to incorrect behavior. Additionally, I've pointed out some areas for improvement regarding code practices, such as avoiding sys.path manipulation and removing debug statements. Overall, the changes are good, but these key issues should be fixed to ensure the stability and maintainability of the new features.
| **{"metadata": {}} # Pass as additional fields | ||
| ) | ||
|
|
||
| _color_log(f"🔧 Executing command: {command}", Color.cyan) |
There was a problem hiding this comment.
The function _color_log is called here, but it is not defined in this file. The defined function is color_log (without the leading underscore). This will cause a NameError at runtime.
| _color_log(f"🔧 Executing command: {command}", Color.cyan) | |
| color_log(f"🔧 Executing command: {command}", Color.cyan) |
| async def search_artifact_chunks(self, user_query: str, search_filter: dict =None, top_k: int = None)-> Any: | ||
| if search_filter is None: | ||
| search_filter = {} | ||
| result = await self.retriever.async_search(self.workspace_id, user_query, search_filter, top_k) | ||
| return result |
There was a problem hiding this comment.
The new method search_artifact_chunks calls self.retriever.async_search. However, the retriever attribute is not defined or initialized anywhere in the WorkSpace class. This will result in an AttributeError when this method is called. You need to initialize self.retriever in the __init__ method, likely from a parameter.
|
|
||
| from aworld.tools.human.human import HUMAN | ||
|
|
||
| sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) |
There was a problem hiding this comment.
Modifying sys.path dynamically is generally considered an anti-pattern in Python as it can lead to unpredictable import behavior and makes the project structure harder to understand and maintain. It's better to rely on Python's standard import mechanisms. Given the file structure, you should be able to use relative imports or configure your project's PYTHONPATH to include the project root. Please consider refactoring this to avoid sys.path manipulation.
| for agents_dir in self.inner_plugins_agent_dirs: | ||
| try: | ||
| self.cli.console.print(f"📦 Loading built-in agents from: {agents_dir}") | ||
| init_agents(str(agents_dir)) | ||
| inner_agents = LocalAgentRegistry.list_agents() | ||
| for agent in inner_agents: | ||
| agent_info = AgentInfo.from_source(agent, source_location=str(agents_dir)) | ||
| all_agents_info.append(agent_info) | ||
| except Exception as e: | ||
| self.cli.console.print(f"[dim]⚠️ Failed to load built-in agents from {agents_dir}: {e}[/dim]") |
There was a problem hiding this comment.
The agent loading logic in this loop is flawed. On each iteration, LocalAgentRegistry.list_agents() fetches all agents registered so far, not just the ones from the current agents_dir. This leads to duplicate AgentInfo objects being created and added to all_agents_info. You should modify this to only process newly loaded agents in each iteration. One way to do this is to get the set of agent names before calling init_agents, and then iterate through the agents after init_agents to find the new ones.
| # logger.warning(f"LocalAgent '{agent.name}' is already registered") | ||
| return |
There was a problem hiding this comment.
The change from raising a ValueError to silently returning when a duplicate agent is registered is a key part of the new agent loading priority. However, silently ignoring duplicates can hide potential configuration issues where an agent is unintentionally being shadowed. It would be better to log a warning to inform the user about the duplicate registration.
Please uncomment the warning log. You'll also need to add import logging and logger = logging.getLogger(__name__) at the top of the file.
| # logger.warning(f"LocalAgent '{agent.name}' is already registered") | |
| return | |
| logger.warning(f"LocalAgent '{agent.name}' is already registered, skipping.") | |
| return |
| # Load custom skills from skills directory | ||
| SKILLS_DIR = cur_dir / "skills" | ||
|
|
||
| print(f"agent_config: {cur_dir}") |
There was a problem hiding this comment.
This print() statement appears to be for debugging purposes. It should be removed before merging to keep the console output clean. If this information is needed for logging, consider using the logging module instead.
|
|
||
|
|
||
|
|
||
| logger.warning(f"⚠️ Failed to load sub-agents: {e}, creating Aworld TeamSwarm without sub-agents") | ||
| return TeamSwarm(aworld_agent) |
There was a problem hiding this comment.
This except block contains empty lines, which can be confusing and is not ideal practice. Also, catching a broad Exception is not ideal; it's better to catch more specific exceptions if possible. Please remove the empty lines for better readability.
| logger.warning(f"⚠️ Failed to load sub-agents: {e}, creating Aworld TeamSwarm without sub-agents") | |
| return TeamSwarm(aworld_agent) | |
| except Exception as e: | |
| logger.warning(f"⚠️ Failed to load sub-agents: {e}, creating Aworld TeamSwarm without sub-agents") | |
| return TeamSwarm(aworld_agent) |
No description provided.