feat(memory): workspace-first retrieval priority in AutoMemoryStore#352
Merged
Conversation
query(), search(), and formatForPrompt() now return workspace-scoped entries before global entries. Workspace entries fill the budget first; global entries only appear as fallback when workspace results are insufficient. This ensures project-specific memories take priority over cross-project global memories during prompt injection. Implementation: workspaceFirstMerge() partitions results by scope using the existing entryFiles tracking, then concatenates workspace entries before global entries within the limit. Closes #351 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe change implements workspace-first retrieval in AutoMemoryStore: query and search now classify entries by scope, merge workspace results before global ones while preserving relative order, and apply limits after merging. Added helpers for scope detection and merging, plus tests covering ordering and limit behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java (1)
648-655: Consider returning a defensive copy from subList.
subList()returns a view backed by the original list. While this works correctly in the current usage, returning a copy would provide more consistent behavior for callers.♻️ Suggested improvement
if (limit > 0 && merged.size() > limit) { - return merged.subList(0, limit); + return new ArrayList<>(merged.subList(0, limit)); } - return merged; + return List.copyOf(merged);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java` around lines 648 - 655, The method in AutoMemoryStore currently returns merged.subList(0, limit) which is a view into the backing list; change it to return a defensive copy instead: replace the subList return with new ArrayList<MemoryEntry>(merged.subList(0, limit)) and also consider returning new ArrayList<MemoryEntry>(merged) in the non-truncated path so callers always get an independent list (use the existing merged variable for both changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-memory/src/test/java/dev/aceclaw/memory/AutoMemoryStoreTest.java`:
- Around line 565-576: In the test method
searchReturnsWorkspaceEntriesBeforeGlobal, tighten the result-size check by
replacing the weak assertion assertThat(results).hasSizeGreaterThanOrEqualTo(2)
with an exact assertion assertThat(results).hasSize(2) because you add exactly
two entries via store.add (MemoryEntry.Category.PATTERN) and expect only those
two from store.search; update the assertion so the test fails if unexpected
additional entries are returned.
---
Nitpick comments:
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java`:
- Around line 648-655: The method in AutoMemoryStore currently returns
merged.subList(0, limit) which is a view into the backing list; change it to
return a defensive copy instead: replace the subList return with new
ArrayList<MemoryEntry>(merged.subList(0, limit)) and also consider returning new
ArrayList<MemoryEntry>(merged) in the non-truncated path so callers always get
an independent list (use the existing merged variable for both changes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52de9f5d-1547-4f71-8a73-783008aaabb0
📒 Files selected for processing (2)
aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.javaaceclaw-memory/src/test/java/dev/aceclaw/memory/AutoMemoryStoreTest.java
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java (1)
636-655: Inconsistent mutability of returned list.When
limit > 0 && merged.size() > limit, the method returns an immutableList.copyOf(). Otherwise, it returns the mutableArrayListdirectly. This inconsistency could cause subtle bugs if callers assume immutability.Additionally, per coding guidelines, consider null-guarding the
rankedparameter.Proposed fix
private List<MemoryEntry> workspaceFirstMerge(List<MemoryEntry> ranked, int limit) { + var safe = ranked != null ? ranked : List.<MemoryEntry>of(); var workspace = new ArrayList<MemoryEntry>(); var global = new ArrayList<MemoryEntry>(); - for (var entry : ranked) { + for (var entry : safe) { if (isGlobalEntry(entry)) { global.add(entry); } else { workspace.add(entry); } } var merged = new ArrayList<MemoryEntry>(workspace); merged.addAll(global); if (limit > 0 && merged.size() > limit) { return List.copyOf(merged.subList(0, limit)); } - return merged; + return List.copyOf(merged); }As per coding guidelines: "Guard against null collection parameters before calling methods like
.stream()— use pattern:var safe = list != null ? list : List.of()"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java` around lines 636 - 655, The method workspaceFirstMerge currently returns a mutable ArrayList in one path and an immutable List.copyOf in another and does not guard ranked for null; update workspaceFirstMerge to null-guard the ranked parameter (e.g., var safe = ranked != null ? ranked : List.of()), build the merged list from safe, and always return an immutable list (use List.copyOf(merged) or List.copyOf(merged.subList(0, limit)) when limit applies) so callers get a consistent immutable result while preserving the existing workspace/global ordering and limit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java`:
- Around line 620-626: The isGlobalEntry method reads the non-thread-safe
entryFiles map without synchronization, causing a data race against writers
(add, remove, replaceEntries); to fix, either change entryFiles to a thread-safe
ConcurrentHashMap so reads in isGlobalEntry are safe, or ensure callers acquire
the same accessLock used by mutators (e.g., have query() and search() acquire
accessLock before calling workspaceFirstMerge which calls isGlobalEntry) so all
access to entryFiles is protected; update the constructor or field declaration
if switching to ConcurrentHashMap, or add lock acquisitions around
workspaceFirstMerge/query/search to use accessLock consistently.
---
Nitpick comments:
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java`:
- Around line 636-655: The method workspaceFirstMerge currently returns a
mutable ArrayList in one path and an immutable List.copyOf in another and does
not guard ranked for null; update workspaceFirstMerge to null-guard the ranked
parameter (e.g., var safe = ranked != null ? ranked : List.of()), build the
merged list from safe, and always return an immutable list (use
List.copyOf(merged) or List.copyOf(merged.subList(0, limit)) when limit applies)
so callers get a consistent immutable result while preserving the existing
workspace/global ordering and limit behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c679aa6-2f4b-4eab-b3a5-fa2a26f0588a
📒 Files selected for processing (2)
aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.javaaceclaw-memory/src/test/java/dev/aceclaw/memory/AutoMemoryStoreTest.java
✅ Files skipped from review due to trivial changes (1)
- aceclaw-memory/src/test/java/dev/aceclaw/memory/AutoMemoryStoreTest.java
Comment on lines
+620
to
+626
| /** | ||
| * Returns whether an entry belongs to the global scope (vs workspace-scoped). | ||
| */ | ||
| boolean isGlobalEntry(MemoryEntry entry) { | ||
| String file = entryFiles.get(entry.id()); | ||
| return GLOBAL_FILE.equals(file); | ||
| } |
There was a problem hiding this comment.
Data race on entryFiles HashMap.
This method reads from entryFiles without synchronization, but write operations (add, remove, replaceEntries, etc.) modify it under locks. Since HashMap is not thread-safe, concurrent read/write can cause undefined behavior or ConcurrentModificationException.
Consider either:
- Using
ConcurrentHashMapforentryFiles, or - Acquiring
accessLockinquery()/search()before callingworkspaceFirstMerge().
Option 1: Use ConcurrentHashMap
- private final Map<String, String> entryFiles;
+ private final Map<String, String> entryFiles = new ConcurrentHashMap<>();And in constructor:
- this.entryFiles = new HashMap<>();
+ // entryFiles already initialized as ConcurrentHashMap🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java` around
lines 620 - 626, The isGlobalEntry method reads the non-thread-safe entryFiles
map without synchronization, causing a data race against writers (add, remove,
replaceEntries); to fix, either change entryFiles to a thread-safe
ConcurrentHashMap so reads in isGlobalEntry are safe, or ensure callers acquire
the same accessLock used by mutators (e.g., have query() and search() acquire
accessLock before calling workspaceFirstMerge which calls isGlobalEntry) so all
access to entryFiles is protected; update the constructor or field declaration
if switching to ConcurrentHashMap, or add lock acquisitions around
workspaceFirstMerge/query/search to use accessLock consistently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
AutoMemoryStore loads both workspace and global entries into one list. Previously, query/search treated them equally — global entries could compete with workspace entries for limited prompt slots. Now workspace-specific memories always take priority for project-specific work.
Test plan
Closes #351
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation