fix(memory): toRelative strips workspace root for ROOTED local filesystem paths#1698
fix(memory): toRelative strips workspace root for ROOTED local filesystem paths#1698McClone wants to merge 1 commit into
Conversation
|
McClone seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
McClone
force-pushed
the
fix/toRelative-workspace-path
branch
2 times, most recently
from
e65c603 to
3863742
Compare
…stem paths In LocalFsMode.ROOTED/UNRESTRICTED, glob returns absolute paths (e.g., /data/agent/.agents/memory/2026-06-10.md). The old toRelative() only stripped the leading '/' which broke resolution in readManagedWorkspaceFileUtf8. Make toRelative non-static and strip the workspace root prefix via WorkspaceManager.getWorkspace() when the path is absolute, preserving the '/' fallback for SANDBOXED virtual paths.
McClone
force-pushed
the
fix/toRelative-workspace-path
branch
from
3863742 to
819a19e
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…scope-ai#1698) Patch coverage was 0% — the 5 new lines in toRelative() were unreachable via the existing RemoteFilesystem-based tests. Changed toRelative() from private to package-private and added three test cases that exercise: - absolute path under workspace root (the primary fix path) - absolute path outside workspace root (falls through to old behaviour) - null and relative paths (edge cases)
AgentScopeJavaBot
added
bug
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a small, focused bugfix for MemoryConsolidator.toRelative(). The old implementation naively stripped the leading / from any path, which worked for SANDBOXED virtual paths (e.g. /memory/file.md → memory/file.md) but produced incorrect results for ROOTED local filesystem paths (e.g. /home/user/workspace/memory/file.md → home/user/workspace/memory/file.md). The fix correctly uses java.nio.file.Path to detect absolute paths rooted under the workspace and relativize them. The static-to-instance conversion is necessary and correct. Downstream normalizeRelativePath() already handles OS-specific separators, so no cross-platform issue. The only gap is the lack of a regression test for the exact scenario being fixed.
| if (path == null) { | ||
| return ""; | ||
| } | ||
| Path p = Path.of(path); |
There was a problem hiding this comment.
[nitpick] No regression test is included for the bug being fixed. The existing MemoryConsolidatorFilesystemTest exercises the Remote/InMemoryStore path but never covers the local ROOTED (non-sandboxed, non-namespaced) case where FileInfo.path() is a real absolute filesystem path. Consider adding a unit test that creates a local WorkspaceManager with a @TempDir, seeds a memory/ directory, and asserts that consolidate() reads daily ledgers correctly — or at minimum a package-private test that directly invokes toRelative() with all three path shapes (SANDBOXED /memory/x.md, ROOTED absolute /tmp/ws/memory/x.md, namespace-relative ns/memory/x.md).
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a small, focused bugfix for MemoryConsolidator.toRelative(). The old implementation naively stripped the leading / from any path, which worked for SANDBOXED virtual paths (e.g. /memory/file.md → memory/file.md) but produced incorrect results for ROOTED local filesystem paths (e.g. /home/user/workspace/memory/file.md → home/user/workspace/memory/file.md). The fix correctly uses java.nio.file.Path to detect absolute paths rooted under the workspace and relativize them. The static-to-instance conversion is necessary and correct. Downstream normalizeRelativePath() already handles OS-specific separators, so no cross-platform issue. The only gap is the lack of a regression test for the exact scenario being fixed.
| if (path == null) { | ||
| return ""; | ||
| } | ||
| Path p = Path.of(path); |
There was a problem hiding this comment.
[nitpick] No regression test is included for the bug being fixed. The existing MemoryConsolidatorFilesystemTest exercises the Remote/InMemoryStore path but never covers the local ROOTED (non-sandboxed, non-namespaced) case where FileInfo.path() is a real absolute filesystem path. Consider adding a unit test that creates a local WorkspaceManager with a @TempDir, seeds a memory/ directory, and asserts that consolidate() reads daily ledgers correctly — or at minimum a package-private test that directly invokes toRelative() with all three path shapes (SANDBOXED /memory/x.md, ROOTED absolute /tmp/ws/memory/x.md, namespace-relative ns/memory/x.md).
3 participants
No description provided.