-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent read_file contents from being persisted in ui_messages.json #8691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add sanitization function to strip large file contents from messages - Sanitize messages before saving to ui_messages.json - Add backward compatibility to purge contents from existing messages - Add comprehensive tests for sanitization logic - Extract constants for better maintainability Fixes #8690
Review SummaryI've reviewed the pull request and identified the following issues that should be addressed: Issues to Address
Overall AssessmentThe PR successfully addresses the core issue of preventing read_file contents from bloating ui_messages.json. The implementation is well-tested with comprehensive test coverage. However, there are minor improvements needed around code cleanliness and consistency in the sanitization logic. |
| @@ -0,0 +1,86 @@ | |||
| import type { ClineMessage } from "@roo-code/types" | |||
| import type { ClineSayTool } from "../../shared/ExtensionMessage" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is unused. The ClineSayTool type is never referenced in this file.
| // Handle batch file reads | ||
| if (sanitized.batchFiles && Array.isArray(sanitized.batchFiles)) { | ||
| sanitized.batchFiles = sanitized.batchFiles.map((file: any) => { | ||
| const sanitizedFile = { ...file } | ||
| // Remove the actual file content, keep only metadata | ||
| // Add type checking for content field | ||
| if ("content" in sanitizedFile && typeof sanitizedFile.content === "string") { | ||
| delete sanitizedFile.content | ||
| } | ||
| return sanitizedFile | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent sanitization between single and batch file reads. Single file reads preserve content if ≤100 characters (line 55), but batch file reads always strip all content regardless of size (lines 66-67). This means the same small file would be saved when read individually but stripped when read as part of a batch, leading to inconsistent UI behavior. Consider applying the same length-based threshold to batch files or documenting this intentional difference.
|
|
||
| if (fileExists) { | ||
| return JSON.parse(await fs.readFile(filePath, "utf8")) | ||
| const messages = JSON.parse(await fs.readFile(filePath, "utf8")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap JSON.parse in try/catch to handle corrupt or malformed ui_messages.json files and log the error to prevent crashes.
This comment was generated because it violated a code review rule: irule_PTI8rjtnhwrWq6jS.
Related GitHub Issue
Closes: #8690
Roo Code Task Context (Optional)
This PR was created with assistance from Roo Code.
Description
This PR addresses the storage bloat issue where
read_filetool contents were being unnecessarily persisted inui_messages.json, causing excessive storage usage and potential UI failures (gray screen).Key implementation details:
sanitizeMessagesForUIStorage()function that strips large file contents from messages before savingpurgeFileContentsFromMessages()to clean up existing bloated messages during rehydrationThe solution is non-breaking as:
Test Procedure
Testing performed:
src/core/task-persistence/__tests__/sanitizeMessages.spec.tscovering:To verify the fix:
ui_messages.json- file contents should show[content stripped for storage]instead of full contentPre-Submission Checklist
Screenshots / Videos
Not applicable - this is a backend storage optimization with no UI changes.
Documentation Updates
The fix is transparent to users and developers - it's an internal optimization that doesn't change any public APIs or user-facing behavior.
Additional Notes
This fix significantly reduces storage usage for tasks that read large files. In the reported issue, a
ui_messages.jsonfile that was 183MB can now be reduced to just a few KB while maintaining full functionality.Get in Touch
Available via GitHub for any questions about this PR.
Important
Adds message sanitization to prevent large file contents from being stored in
ui_messages.json, reducing storage bloat and ensuring backward compatibility.sanitizeMessagesForUIStorage()insanitizeMessages.tsto strip large file contents fromreadFiletool messages, preserving metadata.purgeFileContentsFromMessages()for backward compatibility, cleaning up existing messages during rehydration.readTaskMessages()andsaveTaskMessages()intaskMessages.tsto use new sanitization functions.sanitizeMessages.spec.tswith tests for single and batch file reads, non-file messages, and edge cases.CONTENT_TRUNCATION_LENGTHandSTRIPPED_CONTENT_MARKERinsanitizeMessages.tsfor content handling.This description was created by
for d8c2a0f. You can customize this summary. It will automatically update as commits are pushed.