Repository navigation
fix: sanitize tool error results before llm injection - #1635
serrrfirat wants to merge 3 commits into
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
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 enhances the safety and consistency of how tool execution results, particularly errors, are handled before being fed into the Large Language Model (LLM). By routing all tool outputs through a unified sanitization and wrapping process, it mitigates potential injection vulnerabilities and provides the LLM with a more structured and secure representation of tool interactions, improving its ability to interpret and respond to both successful and failed tool calls. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of tool results by centralizing the sanitization and wrapping logic within the process_tool_result function in src/tools/execute.rs. This ensures that both successful tool outputs and error messages are consistently formatted, XML-wrapped, and protected against injection attempts before being passed to the LLM. The src/agent/dispatcher.rs file was updated to utilize this new centralized function, and comprehensive tests were added or modified to verify the correct behavior, including the neutralization of potential injection vulnerabilities in error messages. The review feedback indicates that the documentation for process_tool_result should be updated to clearly state that both success and error paths are now sanitized and wrapped. Additionally, a high-severity concern was raised regarding compliance with Rule 4, which dictates that sanitization should only be applied to data paths sent to external services, not trusted local endpoints; clarification is requested on whether this tool's output is intended for an external service or if the sanitization needs reconsideration.
| @@ -118,7 +118,6 @@ pub async fn execute_tool_with_safety( | |||
| /// Process a tool result into a `ChatMessage::tool_result` with safety sanitization. | |||
| /// | |||
| /// On success: sanitize → wrap → ChatMessage::tool_result. | |||
There was a problem hiding this comment.
The documentation for this function could be improved to reflect the new behavior for handling errors. While the old line about error handling was correctly removed, the current docstring only mentions the success path. It's important to clarify that both success and error results are now sanitized and wrapped. Additionally, please ensure this sanitization aligns with Rule 4: 'Tool output previews sent to trusted local endpoints (e.g., TUI, web gateway) should not be sanitized. Sanitization should only be applied to data paths sent to external services, such as an LLM.' If this tool's output is for a trusted local endpoint, the sanitization should be reconsidered. If it's for an external service, the documentation should explicitly state this context.
| /// On success: sanitize → wrap → ChatMessage::tool_result. | |
| /// On success or error: sanitize → wrap → ChatMessage::tool_result. |
References
- Tool output previews sent to trusted local endpoints (e.g., TUI, web gateway) should not be sanitized. Sanitization should only be applied to data paths sent to external services, such as an LLM.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Closing this because it was based on feat/multi-tenant-isolation-phases-2-4 instead of staging. Replacement PR: #1639 |
Summary
process_tool_result()helper instead of formatting raw tool errors into LLM contextVerification
cargo test test_process_tool_result_error --libcargo test test_tool_error_format_includes_tool_name --libcargo check --lib