diff --git a/packages/tools/src/claude-memory.test.ts b/packages/tools/src/claude-memory.test.ts index c4f62f16c..58bf60ff3 100644 --- a/packages/tools/src/claude-memory.test.ts +++ b/packages/tools/src/claude-memory.test.ts @@ -24,9 +24,9 @@ const FILE_CONTENT = "line1\nline2\nline3\nline4\nline5" function mockDocument(content: string) { // `readFile` matches by `documentId === normalizePathToCustomId(path)`. - // normalizePathToCustomId("/memories/notes.txt") -> "memories_notes_txt" + // normalizePathToCustomId("/memories/notes.txt") -> "memories_snotes_dtxt" searchExecute.mockResolvedValue({ - results: [{ documentId: "memories_notes_txt", content }], + results: [{ documentId: "memories_snotes_dtxt", content }], }) } @@ -97,8 +97,8 @@ describe("ClaudeMemoryTool exact-file matching", () => { it("view finds the exact file even when a neighbour ranks first", async () => { searchExecute.mockResolvedValue({ results: [ - { documentId: "memories_notes_backup_txt", content: "backup stuff" }, - { documentId: "memories_notes_txt", content: FILE_CONTENT }, + { documentId: "memories_snotes__backup_dtxt", content: "backup stuff" }, + { documentId: "memories_snotes_dtxt", content: FILE_CONTENT }, ], }) @@ -117,7 +117,7 @@ describe("ClaudeMemoryTool exact-file matching", () => { // be served as the requested one. searchExecute.mockResolvedValue({ results: [ - { documentId: "memories_notes_backup_txt", content: "backup stuff" }, + { documentId: "memories_snotes__backup_dtxt", content: "backup stuff" }, ], }) @@ -133,7 +133,7 @@ describe("ClaudeMemoryTool exact-file matching", () => { it("str_replace refuses to modify a different file than requested", async () => { searchExecute.mockResolvedValue({ results: [ - { documentId: "memories_notes_backup_txt", content: "backup stuff" }, + { documentId: "memories_snotes__backup_dtxt", content: "backup stuff" }, ], }) @@ -156,7 +156,7 @@ describe("ClaudeMemoryTool str_replace replacement literalness", () => { searchExecute.mockReset() addMock.mockReset() searchExecute.mockResolvedValue({ - results: [{ documentId: "memories_notes_txt", content: FILE_CONTENT }], + results: [{ documentId: "memories_snotes_dtxt", content: FILE_CONTENT }], }) tool = new ClaudeMemoryTool("test-api-key") }) @@ -181,3 +181,80 @@ describe("ClaudeMemoryTool str_replace replacement literalness", () => { expect(stored).not.toContain("line3") }) }) + +describe("ClaudeMemoryTool customId encoding", () => { + let tool: ClaudeMemoryTool + + beforeEach(() => { + searchExecute.mockReset() + addMock.mockReset() + tool = new ClaudeMemoryTool("test-api-key") + }) + + async function customIdFor(path: string) { + addMock.mockReset() + const result = await tool.handleCommand({ + command: "create", + path, + file_text: "contents", + }) + expect(result.success).toBe(true) + return addMock.mock.calls[0]?.[0]?.customId as string + } + + it("gives paths that differ only in their separators distinct customIds", async () => { + // All three collapsed to `memories_notes_txt` under the old encoding, + // so creating one silently overwrote the others. + const ids = [ + await customIdFor("/memories/notes.txt"), + await customIdFor("/memories/notes_txt"), + await customIdFor("/memories/notes/txt"), + await customIdFor("/memories/project/a.md"), + await customIdFor("/memories/project_a.md"), + ] + + expect(new Set(ids).size).toBe(ids.length) + }) + + it("still finds documents written under the legacy customId", async () => { + searchExecute.mockResolvedValue({ + results: [ + { + documentId: "memories_notes_txt", + content: FILE_CONTENT, + metadata: { file_path: FILE_PATH }, + }, + ], + }) + + const result = await tool.handleCommand({ + command: "view", + path: FILE_PATH, + }) + + expect(result.success).toBe(true) + expect(result.content).toContain("line1") + }) + + it("does not serve a legacy document whose stored path differs", async () => { + // `memories_notes_txt` is ambiguous: it could be /memories/notes.txt, + // /memories/notes_txt or /memories/notes/txt. Only the stored path decides. + searchExecute.mockResolvedValue({ + results: [ + { + documentId: "memories_notes_txt", + content: "someone else's file", + metadata: { file_path: "/memories/notes/txt" }, + }, + ], + }) + + const result = await tool.handleCommand({ + command: "view", + path: FILE_PATH, + }) + + expect(result.success).toBe(false) + expect(result.error).toContain("File not found") + }) +}) diff --git a/packages/tools/src/claude-memory.ts b/packages/tools/src/claude-memory.ts index 8c665701a..3676edb36 100644 --- a/packages/tools/src/claude-memory.ts +++ b/packages/tools/src/claude-memory.ts @@ -47,14 +47,32 @@ export class ClaudeMemoryTool { private memoryContainerPrefix: string /** - * Normalize file path to be used as customId - * Converts /memories/file.txt -> memories_file_txt + * Normalize file path to be used as customId. + * Converts /memories/file.txt -> memories_sfile_dtxt + * + * The encoding is reversible: `_` is the escape character, so every `_` in + * the output opens a two-character sequence (`__` = literal `_`, `_s` = `/`, + * `_d` = `.`). Distinct paths therefore always get distinct customIds. The + * previous scheme collapsed `/`, `.` and `_` all to `_`, so + * /memories/notes.txt, /memories/notes_txt and /memories/notes/txt shared + * one customId and silently overwrote each other. */ private normalizePathToCustomId(path: string): string { return path .replace(/^\//, "") // Remove leading slash - .replace(/\//g, "_") // Replace / with _ - .replace(/\./g, "_") // Replace . with _ + .replace(/_/g, "__") // Escape literal _ first + .replace(/\//g, "_s") // / -> _s + .replace(/\./g, "_d") // . -> _d + } + + /** + * The pre-collision-fix customId for a path. Documents written before that + * fix still carry these ids, so reads fall back to them — guarded by an + * exact `file_path` match, since legacy ids are the ambiguous ones. The + * next write to the path promotes the document to the new customId. + */ + private legacyPathToCustomId(path: string): string { + return path.replace(/^\//, "").replace(/\//g, "_").replace(/\./g, "_") } constructor(apiKey: string, config?: ClaudeMemoryConfig) { @@ -576,9 +594,12 @@ export class ClaudeMemoryTool { }> { try { const normalizedId = this.normalizePathToCustomId(filePath) + const legacyId = this.legacyPathToCustomId(filePath) + // Query with the legacy id: it keeps the path words intact, so search + // relevance is unchanged, and it is what pre-fix documents are keyed on. const response = await this.client.search.execute({ - q: normalizedId, + q: legacyId, containerTags: this.containerTags, limit: 5, includeFullDocs: true, @@ -587,9 +608,15 @@ export class ClaudeMemoryTool { // Only accept the exact customId match. Falling back to the top // semantic hit would let callers read — and worse, modify or // delete — a different file than the one they asked for. - const document = response.results?.find( - (r) => r.documentId === normalizedId, - ) + const document = + response.results?.find((r) => r.documentId === normalizedId) ?? + // Documents written before the customId encoding was made + // reversible. Their ids are ambiguous, so require the stored + // path to match exactly. + response.results?.find( + (r) => + r.documentId === legacyId && r.metadata?.file_path === filePath, + ) if (!document) { return { diff --git a/packages/tools/src/tool-operations.test.ts b/packages/tools/src/tool-operations.test.ts index 136a19bea..56d461abe 100644 --- a/packages/tools/src/tool-operations.test.ts +++ b/packages/tools/src/tool-operations.test.ts @@ -176,7 +176,7 @@ describe("memoryForget", () => { describe("ClaudeMemoryTool", () => { const FILE_PATH = "/memories/prefs.txt" - const CUSTOM_ID = "memories_prefs_txt" + const CUSTOM_ID = "memories_sprefs_dtxt" function mockFileDocument(content: string) { searchExecute.mockResolvedValue({ @@ -262,7 +262,7 @@ describe("ClaudeMemoryTool", () => { expect(result.success).toBe(true) expect(clientAdd).toHaveBeenCalledWith( - expect.objectContaining({ customId: "memories_renamed_txt" }), + expect.objectContaining({ customId: "memories_srenamed_dtxt" }), ) expect(documentsDelete).toHaveBeenCalledWith(CUSTOM_ID) })