-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: implement hierarchical memory management system #6603
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 HierarchicalMemoryManager class to recursively load memory files - Update ApiMessage interface with isHierarchicalMemory flag - Integrate memory loading into readFileTool - Add UI controls in ContextManagementSettings - Create HierarchicalMemoryModal for viewing loaded memories - Add memory view button to TaskActions - Implement context compression compatibility - Add comprehensive tests for HierarchicalMemoryManager - Add translation keys for new UI elements Fixes #6602
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.
I reviewed my own code and found bugs I didn't know I was capable of writing.
| if (exists) { | ||
| const body = await fs.readFile(full, "utf8") | ||
| messages.push({ | ||
| role: "user", |
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.
Is this intentional? The hierarchical memory messages are created with role "user" but the issue specification shows "system" role in the examples. This could affect how the AI interprets these memory instructions.
| this.read.add(full) | ||
| } | ||
| } catch (e: any) { | ||
| if (e.code !== "ENOENT") console.error(e) |
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.
Potential memory leak: The read Set never gets cleared except when explicitly calling clearCache(). For long-running tasks, this could accumulate many file paths. Consider:
| if (e.code !== "ENOENT") console.error(e) | |
| } catch (e: any) { | |
| if (e.code !== "ENOENT") { | |
| console.error(`Failed to read memory file ${full}:`, e) | |
| } | |
| } |
Also, should we implement a size limit or clear the cache when the task ends?
| } = state ?? {} | ||
|
|
||
| // Load hierarchical memory for approved files | ||
| if (cline.memoryManager) { |
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.
Race condition risk: Multiple approved files trigger memory loading sequentially. If files are in the same directory, the timing could cause issues. Consider collecting unique directories first:
| if (cline.memoryManager) { | |
| // Load hierarchical memory for approved files | |
| if (cline.memoryManager) { | |
| const approvedFiles = fileResults.filter((result) => result.status === "approved") | |
| const uniqueDirs = new Set<string>() | |
| for (const fileResult of approvedFiles) { | |
| const fullPath = path.resolve(cline.cwd, fileResult.path) | |
| uniqueeDirs.add(path.dirname(fullPath)) | |
| } | |
| for (const dir of uniqueDirs) { | |
| const memoryMessages = await cline.memoryManager.loadFor(dir, cline.cwd) | |
| if (memoryMessages.length > 0) { | |
| await cline.injectHierarchicalMemory(memoryMessages) | |
| } | |
| } | |
| } |
| expect(result).toHaveLength(1) | ||
| }) | ||
|
|
||
| it.skip("should handle Windows-style paths", async () => { |
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.
The Windows path test is skipped. Could we add proper cross-platform path handling tests? This is important for Windows users.
| <h3 className="text-sm font-medium mb-2 text-vscode-foreground"> | ||
| {t("chat:hierarchicalMemory.loadedFiles")} | ||
| </h3> | ||
| <div className="space-y-1 overflow-y-auto max-h-[calc(80vh-200px)]"> |
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.
UI enhancement opportunity: When many memory files are loaded, users might benefit from a search/filter feature. Consider adding a simple text filter above the file list?
|
Closing, not on the roadmap |
This PR implements a hierarchical memory management system as requested in issue #6602. The feature allows Roo Code to automatically load memory files (like CLAUDE.md or Roorules.md) recursively from parent directories when reading files.
Summary of Changes
Core Implementation
isHierarchicalMemoryflag to distinguish memory messages from regular messagesIntegration Points
UI Components
Configuration
enableHierarchicalMemoryandhierarchicalMemoryFileNamessettingsTesting
How It Works
readFileTool, the system checks if hierarchical memory is enabledisHierarchicalMemory: trueBenefits
Fixes #6602
Important
Implements a hierarchical memory management system with new classes, UI components, and settings, enhancing context management by loading memory files recursively from parent directories.
HierarchicalMemoryManagerclass for recursive directory walking and memory file loading.ApiMessagewithisHierarchicalMemoryflag.readFileTool()after file approval.Taskclass.ContextManagementSettingsfor toggle and file name configuration.HierarchicalMemoryModalfor viewing loaded memory files.TaskActionswith a memory view button.enableHierarchicalMemoryandhierarchicalMemoryFileNamesto global settings.HierarchicalMemoryManagercovering duplicates, errors, and edge cases.This description was created by
for a2e6ec5. You can customize this summary. It will automatically update as commits are pushed.