Conversation
/cost support by agent statistics pre [aworld_agent]: use CONTEXT_TOOL to fetch past sessions and thire path [core]: filesystem based memory
Summary of ChangesHello, 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 AWorld CLI by introducing context compression capabilities, refining token cost tracking with agent-specific statistics, and migrating the underlying memory storage to a more robust filesystem-based approach. These changes aim to provide users with greater control over conversation context, improve resource management, and offer better insights into agent operations, while also enabling agents to leverage historical session data more effectively. 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. 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
The pull request introduces a new context optimization feature with a LIMIT_STRATEGY for token limits, enhances history tracking with agent-specific token statistics, and switches the default memory store to a new FileSystemMemoryStore. However, the review highlights several issues: memory.get_all is incorrectly called without await in run_context_optimization, the context optimization feature is incomplete as it doesn't merge file context as described, and the FileSystemMemoryStore.history method's implementation doesn't match its docstring. Additionally, there are concerns about breaking API changes in video_creation, grep_search, glob_search, and read_file tool parameters, a violation of encapsulation by calling a private method in aworld-cli/main.py, and a reduction in logging severity for potentially critical errors in ast_analyzer.py.
| } | ||
| # `get_all` is a synchronous method in the current memory implementations, | ||
| # so we must not `await` it. | ||
| all_items = memory.get_all(filters=filters) |
There was a problem hiding this comment.
The comment states that get_all is a synchronous method and should not be awaited. However, AworldMemory.get_all (from aworld/memory/main.py) is an async method and should be awaited to ensure proper execution and prevent potential runtime errors or unexpected behavior. This applies to line 284 as well.
| all_items = memory.get_all(filters=filters) | |
| all_items = await memory.get_all(filters=filters) |
| agent_memory_config=agent_memory_config, | ||
| ) | ||
| # Get the newly generated summary | ||
| summary_items = memory.get_all( |
There was a problem hiding this comment.
Similar to line 254, memory.get_all is an async method and should be awaited to ensure proper execution and prevent potential runtime errors or unexpected behavior.
| summary_items = memory.get_all( | |
| summary_items = await memory.get_all( |
| summary_content = summary_items[-1].content or "" | ||
|
|
||
| # Step 4: Merge both results and write to agent history via _add | ||
| combined_content = summary_content |
There was a problem hiding this comment.
The docstring for run_context_optimization mentions that it will "Merge both results" (history summary and file context). However, combined_content is currently assigned only summary_content, and file_context_content (extracted on line 258) is not included in the final combined content. This means the file context is extracted but not utilized in the compression, which is an incomplete implementation of the stated goal.
combined_content = f"""History Summary:
{summary_content}
File Context:
{file_context_content}"""| result: Optional[List[MemoryItem]] = None | ||
| self._log_timing("history", start_time, success=True, extra=extra) | ||
| return result |
There was a problem hiding this comment.
The history method's docstring indicates it should return List[MemoryItem], but the current implementation returns None and logs success. This suggests the method is either incomplete or incorrectly implemented according to its contract. It should either return the actual history or the docstring should be updated to reflect its current behavior.
| result: Optional[List[MemoryItem]] = None | |
| self._log_timing("history", start_time, success=True, extra=extra) | |
| return result | |
| try: | |
| # TODO: Implement actual history retrieval logic if needed. | |
| # For now, it's a placeholder as filesystem store doesn't track versions per item. | |
| result: Optional[List[MemoryItem]] = [] # Return empty list if no history is tracked | |
| self._log_timing("history", start_time, success=True, extra=extra) | |
| return result |
| cli = AWorldCLI() | ||
| cli._display_conf_info() |
There was a problem hiding this comment.
Calling a private method _display_conf_info directly from _show_banner breaks encapsulation. It would be better to either make _display_conf_info a public method if it's intended for external use, or refactor _show_banner to gather the necessary information directly or through a public interface of AWorldCLI.
| - `content`: Required. The video generation prompt (text description of what to create). | ||
| - `info`: Optional JSON string. Use when passing image/video params, e.g.: | ||
| {"image_url": "<data_path_or_base64_string>", "reference_images": ["<path1>", "<path2>"], "resolution": "720p", "duration": 5, "fps": 24, "output_dir": "./output"} | ||
| - `info`: Required JSON string. Use when passing image/video params, e.g.: |
There was a problem hiding this comment.
The info parameter for the video_creation tool has been changed from Optional to Required. This is a breaking change in the tool's API contract. Any existing code or agents calling this tool without providing the info parameter will now fail. This change should be clearly communicated to users or other developers.
| import traceback | ||
| tb_str = traceback.format_exc() | ||
| logger.error(f" ❌ Failed to generate {layer_name} layer content: {e}\n{tb_str}") | ||
| logger.debug(f" ❌ Failed to generate {layer_name} layer content: {e}\n{tb_str}") |
There was a problem hiding this comment.
Changing the logging level from logger.error to logger.debug for a failure in generating layer content might hide critical issues. If a layer generation fails, it could indicate a significant problem in context building that should be visible at a higher logging level.
| logger.debug(f" ❌ Failed to generate {layer_name} layer content: {e}\n{tb_str}") | |
| logger.error(f" ❌ Failed to generate {layer_name} layer content: {e}\n{tb_str}") |
| logger.debug(f" ✅ Name match +5.0 points") | ||
|
|
||
| except re.error as e: | ||
| # If regex is invalid, log error and skip |
There was a problem hiding this comment.
Changing the logging level from logger.warning to logger.debug for an "Invalid regex" error might hide important issues. An invalid regex could indicate a problem with the input or the system that should be visible at a warning level.
| # If regex is invalid, log error and skip | |
| logger.warning(f"❌ Invalid regex '{mention_pattern}': {e}") |
| required=True, | ||
| desc="Search path, it's a required parameter" |
There was a problem hiding this comment.
The path parameter for the grep_search action has been changed from required=False to required=True. This is a breaking change for any existing calls to this tool that did not explicitly provide a path. This change should be clearly documented and communicated to users.
| required=True, | ||
| desc="Search path, it's a required parameter" |
There was a problem hiding this comment.
The path parameter for the glob_search action has been changed from required=False to required=True. Similar to grep_search, this is a breaking change for existing calls and should be clearly communicated.
2 participants
/cost support by agent statistics
pre
[aworld_agent]: use CONTEXT_TOOL to fetch past sessions and thire path [core]: filesystem based memory