diff --git a/packages/tools/src/claude-memory.test.ts b/packages/tools/src/claude-memory.test.ts index ea9b5999..477b0481 100644 --- a/packages/tools/src/claude-memory.test.ts +++ b/packages/tools/src/claude-memory.test.ts @@ -26,7 +26,7 @@ vi.mock("supermemory", () => { return { default: Supermemory, Supermemory, NotFoundError: sdk.NotFoundError } }) -import { ClaudeMemoryTool } from "./claude-memory" +import { ClaudeMemoryTool, type MemoryCommand } from "./claude-memory" const FILE_PATH = "/memories/notes.txt" // 5 distinct lines so an off-by-one at either end is observable. @@ -383,6 +383,97 @@ describe("ClaudeMemoryTool path IDs", () => { ) }) + it("never writes /memories/a into the legacy ID of /memories/s/a", async () => { + mockDocuments([ + { id: "memories_s_a", filePath: "/memories/s/a", content: "legacy" }, + ]) + + const result = await tool.handleCommand({ + command: "create", + path: "/memories/a", + file_text: "new", + }) + + expect(result.success).toBe(true) + expect(updateMock).toHaveBeenCalledTimes(1) + expect(updateMock.mock.calls[0]?.[1]).toBe("cm_memories_s_a") + }) + + it("refuses to overwrite a foreign document holding the file's ID", async () => { + mockDocuments([ + { id: "cm_memories_s_a", filePath: "/memories/other", content: "x" }, + ]) + + const result = await tool.handleCommand({ + command: "create", + path: "/memories/a", + file_text: "new", + }) + + expect(result.success).toBe(false) + expect(updateMock).not.toHaveBeenCalled() + }) + + it("renames onto a legacy destination in place", async () => { + mockDocuments([ + { + id: "cm_memories_s_src_d_txt", + filePath: "/memories/src.txt", + content: "moved", + }, + { + id: "memories_dst_txt", + filePath: "/memories/dst.txt", + content: "stale", + }, + ]) + sdk.delete.mockResolvedValue({ count: 1 }) + + const result = await tool.handleCommand({ + command: "rename", + old_path: "/memories/src.txt", + new_path: "/memories/dst.txt", + }) + + expect(result.success).toBe(true) + expect(updateMock).toHaveBeenCalledWith( + expect.anything(), + "memories_dst_txt", + expect.objectContaining({ content: "moved" }), + ) + expect(sdk.delete).toHaveBeenCalledWith(expect.anything(), { + ids: ["cm_memories_s_src_d_txt"], + }) + }) + + it("does not write when the lookup fails with a non-404 error", async () => { + documentsGetMock.mockRejectedValue( + Object.assign(new Error("upstream down"), { status: 500 }), + ) + + const result = await tool.handleCommand({ + command: "create", + path: FILE_PATH, + file_text: "fresh", + }) + + expect(result.success).toBe(false) + expect(result.error).toContain("upstream down") + expect(updateMock).not.toHaveBeenCalled() + expect(sdk.add).not.toHaveBeenCalled() + }) + + it("keeps path required for every command except rename", () => { + // @ts-expect-error view needs a path + const view: MemoryCommand = { command: "view" } + const rename: MemoryCommand = { + command: "rename", + old_path: "/memories/a", + new_path: "/memories/b", + } + expect([view, rename]).toHaveLength(2) + }) + it("ignores a legacy ID that belongs to a different path", async () => { mockDocuments([ { id: "memories_notes_txt", filePath: FILE_PATH, content: "other" }, diff --git a/packages/tools/src/claude-memory.ts b/packages/tools/src/claude-memory.ts index f227eb1e..6ea73f6e 100644 --- a/packages/tools/src/claude-memory.ts +++ b/packages/tools/src/claude-memory.ts @@ -10,10 +10,7 @@ export type ClaudeMemoryConfig = SupermemoryToolsConfig export const CLAUDE_MEMORY_SOURCE = "claude-memory" -export interface MemoryCommand { - command: "view" | "create" | "str_replace" | "insert" | "delete" | "rename" - // every command except rename addresses the file via path - path?: string +interface MemoryCommandFields { // view specific view_range?: [number, number] // create specific @@ -30,6 +27,13 @@ export interface MemoryCommand { new_path?: string } +export type MemoryCommand = + | (MemoryCommandFields & { + command: "view" | "create" | "str_replace" | "insert" | "delete" + path: string + }) + | (MemoryCommandFields & { command: "rename"; path?: string }) + export interface MemoryResponse { success: boolean content?: string @@ -56,13 +60,13 @@ export class ClaudeMemoryTool { private client: Supermemory private namespace: string - /** Reversible encoding so distinct paths never share an ID: /memories/a_b.txt -> memories_s_a__b_d_txt */ + /** Reversible encoding; the `cm_` prefix keeps it disjoint from legacy IDs, which always start with `memories`: /memories/a_b.txt -> cm_memories_s_a__b_d_txt */ private normalizePathToId(path: string): string { - return path + return `cm_${path .replace(/^\//, "") .replace(/_/g, "__") .replace(/\//g, "_s_") - .replace(/\./g, "_d_") + .replace(/\./g, "_d_")}` } /** Pre-3.1.2 IDs, where / and . both became _ */ @@ -334,13 +338,9 @@ export class ClaudeMemoryTool { fileText: string, ): Promise { try { - // Overwrite a legacy-ID file in place instead of leaving a stale duplicate. - const existing = await this.getFileDocument(filePath) - const id = - existing.document?.documentId ?? - existing.documentId ?? - this.normalizePathToId(filePath) - await this.writeFile(id, fileText, { + const target = await this.resolveWriteId(filePath) + if ("error" in target) return { success: false, error: target.error } + await this.writeFile(target.id, fileText, { source: CLAUDE_MEMORY_SOURCE, claude_memory_type: "file", file_path: filePath, @@ -470,14 +470,16 @@ export class ClaudeMemoryTool { try { // Find the document first const readResult = await this.getFileDocument(filePath) - if (!readResult.success || !readResult.document) { + const documentId = + readResult.document?.documentId ?? readResult.documentId + if (!documentId) { return { success: false, error: readResult.error || "File not found", } } - await this.deleteFile(readResult.document.documentId) + await this.deleteFile(documentId) return { success: true, @@ -516,17 +518,17 @@ export class ClaudeMemoryTool { } } - const originalContent = readResult.document.content - const newNormalizedId = this.normalizePathToId(newPath) + // Overwrite the destination's existing document (possibly a legacy ID) so no stale copy survives. + const target = await this.resolveWriteId(newPath) + if ("error" in target) return { success: false, error: target.error } - await this.writeFile(newNormalizedId, originalContent, { + await this.writeFile(target.id, readResult.document.content, { ...readResult.document.metadata, file_path: newPath, last_modified: new Date().toISOString(), }) - // Same normalized ID means the write above already replaced the file in place. - if (readResult.document.documentId !== newNormalizedId) { + if (readResult.document.documentId !== target.id) { await this.deleteFile(readResult.document.documentId) } @@ -577,6 +579,10 @@ export class ClaudeMemoryTool { document?: ClaudeFileDocument /** Set when the file exists, even if its content is still processing. */ documentId?: string + /** Only true for a confirmed 404, never for a failed lookup. */ + notFound?: boolean + /** The file's own ID is held by a document that isn't this file. */ + occupied?: boolean error?: string }> { try { @@ -585,6 +591,7 @@ export class ClaudeMemoryTool { this.normalizePathToId(filePath), this.legacyNormalizePathToId(filePath), ] + let occupied = false for (const id of ids) { let document: Awaited> try { @@ -599,6 +606,7 @@ export class ClaudeMemoryTool { document.metadata?.source !== CLAUDE_MEMORY_SOURCE || this.getDocumentFilePath(document) !== filePath ) { + if (id === ids[0]) occupied = true continue } if (typeof document.content !== "string") { @@ -619,7 +627,12 @@ export class ClaudeMemoryTool { } } - return { success: false, error: `File not found: ${filePath}` } + return { + success: false, + notFound: true, + occupied, + error: `File not found: ${filePath}`, + } } catch (error) { return { success: false, @@ -628,6 +641,22 @@ export class ClaudeMemoryTool { } } + /** The ID a write to filePath must use; errors rather than guessing when the lookup fails. */ + private async resolveWriteId( + filePath: string, + ): Promise<{ id: string } | { error: string }> { + const existing = await this.getFileDocument(filePath) + const existingId = existing.document?.documentId ?? existing.documentId + if (existingId) return { id: existingId } + if (!existing.notFound) { + return { error: existing.error ?? `Failed to look up ${filePath}` } + } + if (existing.occupied) { + return { error: `Another document already uses the ID for ${filePath}` } + } + return { id: this.normalizePathToId(filePath) } + } + private getDocumentFilePath(document: { metadata: unknown }): string | undefined { diff --git a/packages/tools/src/openai/tools.ts b/packages/tools/src/openai/tools.ts index 9bc593b0..9655550b 100644 --- a/packages/tools/src/openai/tools.ts +++ b/packages/tools/src/openai/tools.ts @@ -332,6 +332,9 @@ export function createDocumentDeleteFunction( }: { documentId: string }): Promise { + if (!documentId?.trim()) { + return { success: false, error: "documentId is required" } + } try { await deleteDocument(client, namespace, documentId) @@ -497,6 +500,20 @@ export function createToolCallExecutor( } const args = parsed.value + // Blank arguments are only valid for tools with no required fields. + const schema = + memoryToolSchemas[functionName as keyof typeof memoryToolSchemas] + const missing = schema?.parameters.required.filter( + (field: string) => + typeof args[field] !== "string" || args[field].trim() === "", + ) + if (missing?.length) { + return JSON.stringify({ + success: false, + error: `Missing required arguments for ${functionName}: ${missing.join(", ")}`, + }) + } + switch (functionName) { case "searchMemories": return JSON.stringify(await tools.searchMemories(args)) diff --git a/packages/tools/src/tool-operations.test.ts b/packages/tools/src/tool-operations.test.ts index 8059aa09..ca48d547 100644 --- a/packages/tools/src/tool-operations.test.ts +++ b/packages/tools/src/tool-operations.test.ts @@ -388,6 +388,22 @@ describe("openai executeToolCall argument parsing", () => { expect(sdk.profile).not.toHaveBeenCalled() }) + it.each([ + ["documentDelete", "documentId"], + ["searchMemories", "informationToGet"], + ["addMemory", "memory"], + ])("rejects blank arguments for %s, which requires %s", async (name, field) => { + const execute = openAi.createToolCallExecutor(API_KEY) + + const result = JSON.parse(await execute(toolCall(name, ""))) + + expect(result.success).toBe(false) + expect(result.error).toContain(field) + expect(sdk.documentsDelete).not.toHaveBeenCalled() + expect(sdk.search).not.toHaveBeenCalled() + expect(sdk.add).not.toHaveBeenCalled() + }) + it("still reports malformed JSON as a tool error", async () => { const execute = openAi.createToolCallExecutor(API_KEY) @@ -424,18 +440,25 @@ describe("openai executeToolCall argument parsing", () => { describe("ClaudeMemoryTool", () => { const FILE_PATH = "/memories/prefs.txt" - const FILE_ID = "memories_s_prefs_d_txt" + const FILE_ID = "cm_memories_s_prefs_d_txt" + + beforeEach(() => { + sdk.documentsGet.mockRejectedValue(new sdk.NotFoundError("missing")) + }) function mockFileDocument(content: string) { - sdk.documentsGet.mockResolvedValue({ - id: "doc_file_1", - content, - metadata: { - source: "claude-memory", - claude_memory_type: "file", - file_path: FILE_PATH, - }, - system: { status: "done" }, + sdk.documentsGet.mockImplementation(async (_ns: string, id: string) => { + if (id !== FILE_ID) throw new sdk.NotFoundError("missing") + return { + id: "doc_file_1", + content, + metadata: { + source: "claude-memory", + claude_memory_type: "file", + file_path: FILE_PATH, + }, + system: { status: "done" }, + } }) } @@ -578,7 +601,7 @@ describe("ClaudeMemoryTool", () => { expect(result.success).toBe(true) expect(sdk.add).toHaveBeenCalledWith( DEFAULT_NS, - expect.objectContaining({ id: "memories_s_renamed_d_txt" }), + expect.objectContaining({ id: "cm_memories_s_renamed_d_txt" }), ) expect(sdk.documentsDelete).toHaveBeenCalledWith(DEFAULT_NS, { ids: [FILE_ID], diff --git a/packages/tools/test/claude-memory-commands.test.ts b/packages/tools/test/claude-memory-commands.test.ts index 8764cd80..953a2bc3 100644 --- a/packages/tools/test/claude-memory-commands.test.ts +++ b/packages/tools/test/claude-memory-commands.test.ts @@ -25,15 +25,19 @@ vi.mock("supermemory", () => { import { createClaudeMemoryTool } from "../src/claude-memory" function stubFile(path: string, content: string) { - sdk.get.mockImplementation(async () => ({ - id: "doc_src", - metadata: { - source: "claude-memory", - claude_memory_type: "file", - file_path: path, - }, - content, - })) + const fileId = `cm_${path.slice(1).replace(/_/g, "__").replace(/\//g, "_s_").replace(/\./g, "_d_")}` + sdk.get.mockImplementation(async (_ns: string, id: string) => { + if (id !== fileId) throw new sdk.NotFoundError("missing") + return { + id: "doc_src", + metadata: { + source: "claude-memory", + claude_memory_type: "file", + file_path: path, + }, + content, + } + }) } describe("ClaudeMemoryTool rename", () => {