Improving memory tool instructions and eval testing#18091
Conversation
Summary of ChangesHello @alisa-alisa, 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 improves the robustness and clarity of the memory tool's functionality and its testing. It refines the tool's instructions to better guide its usage, while simultaneously expanding the suite of evaluation tests to cover a broader range of memory-related scenarios. A key enhancement is the update to the model output validation utility, which now allows for more precise control over expected and forbidden content in test assertions. 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 significantly improves the save_memory tool by refining its instructional prompt and adding a comprehensive suite of evaluation tests. The refactoring of the validateModelOutput test helper to use a configuration object is a welcome change that improves readability and extensibility, as demonstrated by the addition of forbiddenContent validation. Overall, these changes enhance the tool's reliability and test coverage. I've identified one high-severity issue in a new test case where an assertion should be strengthened to prevent potential regressions.
|
Size Change: -254 B (0%) Total Size: 23.7 MB ℹ️ View Unchanged
|
6a47017 to
d26323d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the save_memory tool by refining its instructions and adding a comprehensive suite of evaluation tests. The refactoring of validateModelOutput into assertModelHasOutput and checkModelOutputContent further enhances test helpers by adhering to the Single Responsibility Principle and supporting forbidden content checks. However, a security audit identified two high-severity vulnerabilities in the memoryTool.ts implementation: a Markdown injection vulnerability in the save_memory tool that allows attackers to inject arbitrary content and instructions into the agent's long-term memory, and a logic flaw in the tool invocation process enabling a "bait and switch" attack where the content written to the memory file can differ from user approval. Both issues could lead to persistent prompt injection. Additionally, an existing negative test case could be made more robust. These critical issues should be addressed before merging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the security and reliability of the save_memory tool by addressing markdown injection and a 'Bait and Switch' logic flaw. While these fixes are well-implemented through input sanitization and content integrity checks, a critical security vulnerability remains: internal tool parameters (modified_by_user, modified_content) can still be injected by the LLM, potentially bypassing the new protections. Additionally, the tool's implementation has been refactored for better self-containment within MemoryToolInvocation, and the test suite has been substantially expanded and refactored. A minor issue with inconsistent sanitization for success messages was also noted.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the security and reliability of the save_memory tool. The fixes for the Markdown Injection and "Bait and Switch" vulnerabilities are well-implemented and correctly address the root causes. Storing the proposed content and using it for the final write operation effectively closes the race condition, and sanitizing the input fact by removing newlines prevents prompt injection via new markdown blocks. The addition of additionalProperties: false to the tool's schema provides crucial defense-in-depth against parameter injection attacks.
The refactoring of the test helpers into assertModelHasOutput and checkModelOutputContent is a great improvement for code clarity and maintainability, and it's applied consistently across the test suite. The new evaluation tests for save_memory are comprehensive and greatly increase confidence in the tool's intended behavior.
Overall, these changes are high-quality and represent a major step forward for the tool's robustness. I have no further comments.
Adding more eval tests for save_memory tool. Adjusting validateModelOutput utility for eval tests so that it can accept forbiddenContent too now.
…putContent to follow the Single Responsibility Principle.
- Markdown injection vulnerability - "bait and switch" attack vulnerability
ec939ad to
3ddea17
Compare
Summary
Improves instructions for memory tool and adds more evals to validate new behavior.
Details
Evals updates
validateModelOutputis refactored to include forbiddenContent.validateModelOutputis split intoassertModelHasOutputandcheckModelOutputContentto follow the Single Responsibility Principle.Security vulnerabilities in memory tool
Some security vulnerabilities are fixed as per
/gemini reviewMarkdown Injection
The save_memory tool wasn't properly sanitizing the fact it was asked to remember. An attacker could provide a fact containing Markdown, like a new header (## New Instructions), which would be written directly into your memory file (GEMINI.md). When the agent reads this file for context in the future, it would treat these injected lines as trusted instructions, leading to a persistent prompt injection attack.
Bait and Switch
The process for saving a memory had a logic flaw.
This creates a vulnerability. If the tool could be tricked into using a different fact during the final write than it used for the initial approval screen, it could save malicious content that you never approved. The fix is to ensure the exact content shown during approval is what gets written to the file.
Param injection
Fix:
Related Issues
Fixes #15678
How to Validate
I asked gemini cli to run tests 50 times and validate the tool call and then 20 times and validate the model output. It passed.
Also new tests were very flaky before the prompt refactoring.
Pre-Merge Checklist