diff --git a/apps/docs/integrations/claude-memory.mdx b/apps/docs/integrations/claude-memory.mdx index 39ecacd5..3bee1fc0 100644 --- a/apps/docs/integrations/claude-memory.mdx +++ b/apps/docs/integrations/claude-memory.mdx @@ -182,7 +182,7 @@ Paths are normalized for storage: `/memories/preferences` is stored as `--memori ```typescript { command: "rename", - path: "/memories/old-name.txt", + old_path: "/memories/old-name.txt", new_path: "/memories/new-name.txt" } ``` diff --git a/packages/tools/README.md b/packages/tools/README.md index f6596746..1bc677d2 100644 --- a/packages/tools/README.md +++ b/packages/tools/README.md @@ -288,7 +288,7 @@ The `MemoryPromptData` object provides: #### OpenAI Middleware with Supermemory -The `withSupermemory` function creates an OpenAI client with SuperMemory middleware automatically injected: +The `withSupermemory` function creates an isolated middleware facade around an OpenAI client. It does not mutate the supplied client, so a shared base client can safely be wrapped with different user or conversation options: ```typescript import { withSupermemory } from "@supermemory/tools/openai" diff --git a/packages/tools/package.json b/packages/tools/package.json index 8e9fb55c..63d6de35 100644 --- a/packages/tools/package.json +++ b/packages/tools/package.json @@ -1,7 +1,7 @@ { "name": "@supermemory/tools", "type": "module", - "version": "3.1.1", + "version": "3.1.2", "description": "Memory tools for AI SDK, OpenAI, Voltagent and Mastra with supermemory", "scripts": { "build": "tsdown", diff --git a/packages/tools/src/claude-memory.test.ts b/packages/tools/src/claude-memory.test.ts index 97bfeda5..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. @@ -188,6 +188,80 @@ describe("ClaudeMemoryTool path collisions", () => { }) }) +describe("ClaudeMemoryTool insert line semantics", () => { + let tool: ClaudeMemoryTool + + beforeEach(() => { + documentsGetMock.mockReset() + updateMock.mockReset().mockResolvedValue({ id: "doc", status: "queued" }) + mockDocument(FILE_CONTENT) + tool = new ClaudeMemoryTool("test-api-key") + }) + + // The memory_20250818 spec: insert_text is inserted AFTER line insert_line, + // 0 inserts at the beginning of the file, and the valid range is [0, n_lines]. + + it("insert_line: 0 inserts at the beginning of the file", async () => { + const result = await tool.handleCommand({ + command: "insert", + path: FILE_PATH, + insert_line: 0, + insert_text: "header", + }) + + expect(result.success).toBe(true) + const stored = updateMock.mock.calls[0]?.[2]?.content as string + expect(stored).toBe("header\nline1\nline2\nline3\nline4\nline5") + }) + + it("inserts AFTER the given line, not before it", async () => { + const result = await tool.handleCommand({ + command: "insert", + path: FILE_PATH, + insert_line: 2, + insert_text: "after2", + }) + + expect(result.success).toBe(true) + const stored = updateMock.mock.calls[0]?.[2]?.content as string + // Regression guard: the old 1-based insert-BEFORE landed this one line early. + expect(stored).toBe("line1\nline2\nafter2\nline3\nline4\nline5") + }) + + it("insert_line: n_lines appends at the end of the file", async () => { + const result = await tool.handleCommand({ + command: "insert", + path: FILE_PATH, + insert_line: 5, + insert_text: "tail", + }) + + expect(result.success).toBe(true) + const stored = updateMock.mock.calls[0]?.[2]?.content as string + expect(stored).toBe("line1\nline2\nline3\nline4\nline5\ntail") + }) + + it("rejects insert_line outside [0, n_lines] without writing", async () => { + const below = await tool.handleCommand({ + command: "insert", + path: FILE_PATH, + insert_line: -1, + insert_text: "x", + }) + expect(below.success).toBe(false) + expect(below.error).toContain("[0, 5]") + + const above = await tool.handleCommand({ + command: "insert", + path: FILE_PATH, + insert_line: 6, + insert_text: "x", + }) + expect(above.success).toBe(false) + expect(updateMock).not.toHaveBeenCalled() + }) +}) + describe("ClaudeMemoryTool str_replace replacement literalness", () => { let tool: ClaudeMemoryTool @@ -217,3 +291,198 @@ describe("ClaudeMemoryTool str_replace replacement literalness", () => { expect(stored).toContain(`price is ${dollarSequence} today`) }) }) + +describe("ClaudeMemoryTool path traversal", () => { + let tool: ClaudeMemoryTool + + beforeEach(() => { + documentsGetMock.mockReset() + mockDocument(FILE_CONTENT) + tool = new ClaudeMemoryTool("test-api-key") + }) + + it.each([ + "/memories/..", + "/memories/foo/..", + "/memories/../secrets.txt", + ])("rejects parent-directory path %s", async (path) => { + const result = await tool.handleCommand({ + command: "view", + path, + }) + + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid path") + expect(documentsGetMock).not.toHaveBeenCalled() + }) +}) + +describe("ClaudeMemoryTool path IDs", () => { + let tool: ClaudeMemoryTool + + beforeEach(() => { + documentsGetMock.mockReset() + updateMock.mockReset().mockResolvedValue({ id: "doc", status: "queued" }) + mockDocuments([]) + tool = new ClaudeMemoryTool("test-api-key") + }) + + it("gives paths that used to collide distinct IDs", async () => { + const paths = [ + "/memories/notes.txt", + "/memories/notes_txt", + "/memories/notes/txt", + ] + for (const path of paths) { + await tool.handleCommand({ command: "create", path, file_text: "x" }) + } + + const ids = updateMock.mock.calls.map((call) => call[1]) + expect(new Set(ids).size).toBe(paths.length) + }) + + it("reads and overwrites a file stored under its legacy ID", async () => { + mockDocuments([ + { id: "memories_notes_txt", filePath: FILE_PATH, content: "legacy" }, + ]) + + const view = await tool.handleCommand({ command: "view", path: FILE_PATH }) + expect(view.success).toBe(true) + expect(view.content).toContain("legacy") + + await tool.handleCommand({ + command: "create", + path: FILE_PATH, + file_text: "fresh", + }) + expect(updateMock).toHaveBeenCalledWith( + expect.anything(), + "memories_notes_txt", + expect.objectContaining({ content: "fresh" }), + ) + }) + + it("overwrites a legacy file whose content is still processing", async () => { + mockDocuments([ + { + id: "memories_notes_txt", + filePath: FILE_PATH, + content: null as unknown as string, + }, + ]) + + await tool.handleCommand({ + command: "create", + path: FILE_PATH, + file_text: "fresh", + }) + expect(updateMock).toHaveBeenCalledWith( + expect.anything(), + "memories_notes_txt", + expect.objectContaining({ content: "fresh" }), + ) + }) + + 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" }, + ]) + + const result = await tool.handleCommand({ + command: "view", + path: "/memories/notes_txt", + }) + expect(result.success).toBe(false) + }) +}) diff --git a/packages/tools/src/claude-memory.ts b/packages/tools/src/claude-memory.ts index 7b5d1d11..6ea73f6e 100644 --- a/packages/tools/src/claude-memory.ts +++ b/packages/tools/src/claude-memory.ts @@ -10,9 +10,7 @@ export type ClaudeMemoryConfig = SupermemoryToolsConfig export const CLAUDE_MEMORY_SOURCE = "claude-memory" -export interface MemoryCommand { - command: "view" | "create" | "str_replace" | "insert" | "delete" | "rename" - path: string +interface MemoryCommandFields { // view specific view_range?: [number, number] // create specific @@ -23,10 +21,19 @@ export interface MemoryCommand { // insert specific insert_line?: number insert_text?: string - // rename specific + // rename specific: Claude sends old_path/new_path (path is accepted too + // for backwards compatibility with earlier callers) + old_path?: string 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 @@ -53,8 +60,17 @@ export class ClaudeMemoryTool { private client: Supermemory private namespace: string - /** Converts /memories/file.txt -> memories_file_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 `cm_${path + .replace(/^\//, "") + .replace(/_/g, "__") + .replace(/\//g, "_s_") + .replace(/\./g, "_d_")}` + } + + /** Pre-3.1.2 IDs, where / and . both became _ */ + private legacyNormalizePathToId(path: string): string { return path.replace(/^\//, "").replace(/\//g, "_").replace(/\./g, "_") } @@ -68,17 +84,24 @@ export class ClaudeMemoryTool { */ async handleCommand(command: MemoryCommand): Promise { try { + // rename is the one command that doesn't use `path`: Claude sends + // old_path/new_path. Fall back to `path` so older callers keep working. + const path = + command.command === "rename" + ? (command.old_path ?? command.path) + : command.path + // Validate path security - if (!this.isValidPath(command.path)) { + if (path === undefined || !this.isValidPath(path)) { return { success: false, - error: `Invalid path: ${command.path}. All paths must start with /memories/`, + error: `Invalid path: ${path}. All paths must start with /memories/`, } } switch (command.command) { case "view": - return await this.view(command.path, command.view_range) + return await this.view(path, command.view_range) case "create": if (!command.file_text) { return { @@ -86,22 +109,16 @@ export class ClaudeMemoryTool { error: "file_text is required for create command", } } - return await this.create(command.path, command.file_text) + return await this.create(path, command.file_text) case "str_replace": - // new_str may legitimately be "" (deleting text), so only reject - // when it is missing entirely. old_str must be non-empty — replacing - // the empty string would prepend instead of replacing. + // new_str may be "" (deleting text) but must be present. if (!command.old_str || command.new_str === undefined) { return { success: false, error: "old_str and new_str are required for str_replace command", } } - return await this.strReplace( - command.path, - command.old_str, - command.new_str, - ) + return await this.strReplace(path, command.old_str, command.new_str) case "insert": // insert_text may be "" (inserting a blank line). if ( @@ -115,12 +132,12 @@ export class ClaudeMemoryTool { } } return await this.insert( - command.path, + path, command.insert_line, command.insert_text, ) case "delete": - return await this.delete(command.path) + return await this.delete(path) case "rename": if (!command.new_path) { return { @@ -128,7 +145,7 @@ export class ClaudeMemoryTool { error: "new_path is required for rename command", } } - return await this.rename(command.path, command.new_path) + return await this.rename(path, command.new_path) default: return { success: false, @@ -321,7 +338,9 @@ export class ClaudeMemoryTool { fileText: string, ): Promise { try { - await this.writeFile(this.normalizePathToId(filePath), 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, @@ -414,16 +433,16 @@ export class ClaudeMemoryTool { const originalContent = readResult.document.content const lines = originalContent.split("\n") - // Validate line number - if (insertLine < 1 || insertLine > lines.length + 1) { + // insert_line is the line the text goes after: 0 means the beginning + // of the file and lines.length appends at the end. + if (insertLine < 0 || insertLine > lines.length) { return { success: false, - error: `Invalid line number: ${insertLine}. File has ${lines.length} lines.`, + error: `Invalid insert_line parameter: ${insertLine}. It should be within the range of lines of the file: [0, ${lines.length}]`, } } - // Insert the text (insertLine is 1-based) - lines.splice(insertLine - 1, 0, insertText) + lines.splice(insertLine, 0, insertText) const newContent = lines.join("\n") await this.writeFile(readResult.document.documentId, newContent, { @@ -434,7 +453,7 @@ export class ClaudeMemoryTool { return { success: true, - content: `Text inserted at line ${insertLine} in file: ${filePath}`, + content: `Text inserted after line ${insertLine} in file: ${filePath}`, } } catch (error) { return { @@ -451,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, @@ -497,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) } @@ -556,41 +577,61 @@ export class ClaudeMemoryTool { private async getFileDocument(filePath: string): Promise<{ success: boolean 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 { - const id = this.normalizePathToId(filePath) - let document: Awaited> - try { - document = await this.client.documents.get(this.namespace, id) - } catch (error) { - if (isNotFoundError(error)) { - return { success: false, error: `File not found: ${filePath}` } + // Files written before 3.1.2 still live under the legacy ID. + const ids = [ + this.normalizePathToId(filePath), + this.legacyNormalizePathToId(filePath), + ] + let occupied = false + for (const id of ids) { + let document: Awaited> + try { + document = await this.client.documents.get(this.namespace, id) + } catch (error) { + if (isNotFoundError(error)) continue + throw error + } + + // Different paths can share a legacy ID; only the exact file counts. + if ( + document.metadata?.source !== CLAUDE_MEMORY_SOURCE || + this.getDocumentFilePath(document) !== filePath + ) { + if (id === ids[0]) occupied = true + continue + } + if (typeof document.content !== "string") { + return { + success: false, + documentId: id, + error: `File content unavailable: ${filePath}`, + } } - throw error - } - // Different paths can normalize to the same ID; only the exact file counts. - if ( - document.metadata?.source !== CLAUDE_MEMORY_SOURCE || - this.getDocumentFilePath(document) !== filePath - ) { - return { success: false, error: `File not found: ${filePath}` } - } - if (typeof document.content !== "string") { return { - success: false, - error: `File content unavailable: ${filePath}`, + success: true, + document: { + documentId: id, + content: document.content, + metadata: document.metadata as ClaudeFileMetadata, + }, } } return { - success: true, - document: { - documentId: id, - content: document.content, - metadata: document.metadata as ClaudeFileMetadata, - }, + success: false, + notFound: true, + occupied, + error: `File not found: ${filePath}`, } } catch (error) { return { @@ -600,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 { @@ -618,11 +675,15 @@ export class ClaudeMemoryTool { * Validate that path starts with /memories for security */ private isValidPath(path: string): boolean { - return ( - (path.startsWith("/memories/") || path === "/memories") && - !path.includes("../") && - !path.includes("..\\") - ) + if (!(path.startsWith("/memories/") || path === "/memories")) { + return false + } + if (path.includes("..\\")) { + return false + } + // Reject any parent-directory segment, including trailing "/.." which + // the previous "../" substring check missed (e.g. "/memories/.."). + return !path.split("/").some((segment) => segment === "..") } } diff --git a/packages/tools/src/openai/index.ts b/packages/tools/src/openai/index.ts index 1523bf73..d229a4ed 100644 --- a/packages/tools/src/openai/index.ts +++ b/packages/tools/src/openai/index.ts @@ -23,7 +23,7 @@ import { * @param options.addMemory - Optional mode for memory addition: "always" (default), "never" * @param options.apiKey - Optional Supermemory API key; falls back to SUPERMEMORY_API_KEY * - * @returns An OpenAI client with SuperMemory middleware injected for both Chat Completions and Responses APIs + * @returns A distinct OpenAI middleware facade for Chat Completions and Responses APIs. The supplied client is not mutated. * * @example * ```typescript diff --git a/packages/tools/src/openai/middleware.ts b/packages/tools/src/openai/middleware.ts index 82326102..5c76a78e 100644 --- a/packages/tools/src/openai/middleware.ts +++ b/packages/tools/src/openai/middleware.ts @@ -19,6 +19,34 @@ import { deduplicateMemoriesForMode } from "../tools-shared" import { createLogger, type Logger } from "../vercel/logger" import { convertProfileToMarkdown } from "../vercel/util" +// Keep canonicalization stable across duplicate package copies and hot reloads. +const BASE_CLIENT_SYMBOL = Symbol.for("@supermemory/tools/openai/base-client") +const baseClientByWrapper = new WeakMap() + +const getBaseClient = (client: OpenAI) => + (Reflect.get(client, BASE_CLIENT_SYMBOL) as OpenAI | undefined) ?? + baseClientByWrapper.get(client) ?? + client + +const cloneWithOverrides = ( + source: T, + overrides: Partial, +): T => { + const descriptors = Object.getOwnPropertyDescriptors(source) + + for (const key of Reflect.ownKeys(overrides) as Array) { + const current = Object.getOwnPropertyDescriptor(source, key) + Reflect.set(descriptors, key, { + configurable: current?.configurable ?? true, + enumerable: current?.enumerable ?? false, + value: overrides[key], + writable: current && "writable" in current ? current.writable : true, + }) + } + + return Object.create(Object.getPrototypeOf(source), descriptors) as T +} + const normalizeBaseUrl = (url?: string): string => { const defaultUrl = "https://api.supermemory.ai" return url?.trim().replace(/\/+$/, "") || defaultUrl @@ -658,6 +686,11 @@ export function createOpenAIMiddleware( namespace: string, options?: OpenAIMiddlewareOptions, ) { + const baseClient = getBaseClient(openaiClient) + const baseChat = baseClient.chat + const baseCompletions = baseChat.completions + const baseResponses = baseClient.responses + const logger = createLogger(options?.verbose ?? false) const apiKey = options?.apiKey?.trim() || process.env.SUPERMEMORY_API_KEY?.trim() || "" @@ -676,8 +709,8 @@ export function createOpenAIMiddleware( const mode = options?.mode ?? "profile" const addMemory = options?.addMemory ?? "always" - const originalCreate = openaiClient.chat.completions.create - const originalResponsesCreate = openaiClient.responses?.create + const originalCreate = baseCompletions.create + const originalResponsesCreate = baseResponses?.create /** * Searches for memories and formats them for injection into API calls. @@ -818,7 +851,7 @@ export function createOpenAIMiddleware( } return { request: originalResponsesCreate.call( - openaiClient.responses, + baseResponses, cleanedParams, requestOptions, ), @@ -864,7 +897,7 @@ export function createOpenAIMiddleware( return { request: originalResponsesCreate.call( - openaiClient.responses, + baseResponses, { ...params, input: cleanedInput, @@ -913,7 +946,7 @@ export function createOpenAIMiddleware( logger.debug("No textual user message found, skipping memory search") return { request: originalCreate.call( - openaiClient.chat.completions, + baseCompletions, { ...params, messages: updateChatMemoryContexts(messages), @@ -968,7 +1001,7 @@ export function createOpenAIMiddleware( return { request: originalCreate.call( - openaiClient.chat.completions, + baseCompletions, { ...params, messages: enhancedMessages, @@ -983,14 +1016,26 @@ export function createOpenAIMiddleware( requestOptions?: OpenAI.RequestOptions, ) => deferAPIPromise(() => prepareCreateWithMemory(params, requestOptions)) - openaiClient.chat.completions.create = - createWithMemory as typeof originalCreate + const wrappedCompletions = cloneWithOverrides(baseCompletions, { + create: createWithMemory as typeof originalCreate, + }) + const wrappedChat = cloneWithOverrides(baseChat, { + completions: wrappedCompletions, + }) + const wrappedResponses = + baseResponses && originalResponsesCreate + ? cloneWithOverrides(baseResponses, { + create: createResponsesWithMemory as typeof originalResponsesCreate, + }) + : undefined + const wrappedClient = cloneWithOverrides(baseClient, { + chat: wrappedChat, + ...(wrappedResponses ? { responses: wrappedResponses } : {}), + }) - // Wrap Responses API if available - if (originalResponsesCreate) { - openaiClient.responses.create = - createResponsesWithMemory as typeof originalResponsesCreate - } - - return openaiClient + Object.defineProperty(wrappedClient, BASE_CLIENT_SYMBOL, { + value: baseClient, + }) + baseClientByWrapper.set(wrappedClient, baseClient) + return wrappedClient } diff --git a/packages/tools/src/openai/tools.ts b/packages/tools/src/openai/tools.ts index 5069a615..fec1536f 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) @@ -458,8 +461,18 @@ export function getToolDefinitions(): OpenAI.Chat.Completions.ChatCompletionTool } function parseToolArguments(argumentsJson: string) { + // OpenAI sends "" for tools called with no arguments. + const source = argumentsJson?.trim() || "{}" + try { - return { success: true as const, value: JSON.parse(argumentsJson) } + const value = JSON.parse(source) + + // "null", "5" and "[]" parse but would throw when destructured. + if (typeof value !== "object" || value === null || Array.isArray(value)) { + return { success: false as const } + } + + return { success: true as const, value } } catch { return { success: false as const } } @@ -487,6 +500,22 @@ export function createToolCallExecutor( } const args = parsed.value + // Blank arguments are only valid for tools with no required fields. + // hasOwn so names like "constructor" fall through to the unknown-function error. + const schema = Object.hasOwn(memoryToolSchemas, functionName) + ? memoryToolSchemas[functionName as keyof typeof memoryToolSchemas] + : undefined + 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/shared/cache.ts b/packages/tools/src/shared/cache.ts index e27407b2..d382bb1a 100644 --- a/packages/tools/src/shared/cache.ts +++ b/packages/tools/src/shared/cache.ts @@ -5,6 +5,9 @@ import type { MemoryMode } from "./types" * Generic memory cache for storing per-turn memories to avoid redundant API calls. * Used to cache memory retrieval results during tool-call loops within the same turn. */ +const escapeKeySegment = (segment: string): string => + segment.replace(/%/g, "%25").replace(/:/g, "%3A") + export class MemoryCache { private cache: LRUCache = new LRUCache({ max: 100 }) @@ -25,7 +28,9 @@ export class MemoryCache { message: string, ): string { const normalizedMessage = message.trim().replace(/\s+/g, " ") - return `${namespace}:${threadId || ""}:${mode}:${normalizedMessage}` + const safeNamespace = escapeKeySegment(namespace) + const safeThreadId = escapeKeySegment(threadId ?? "") + return `${safeNamespace}:${safeThreadId}:${mode}:${normalizedMessage}` } /** diff --git a/packages/tools/src/shared/context.ts b/packages/tools/src/shared/context.ts index f98bd265..8521eb5a 100644 --- a/packages/tools/src/shared/context.ts +++ b/packages/tools/src/shared/context.ts @@ -8,8 +8,8 @@ import { Supermemory } from "supermemory" */ export const normalizeBaseUrl = (url?: string): string => { const defaultUrl = "https://api.supermemory.ai" - if (!url) return defaultUrl - return url.endsWith("/") ? url.slice(0, -1) : url + const trimmed = url?.trim().replace(/\/+$/, "") + return trimmed || defaultUrl } /** diff --git a/packages/tools/src/tool-operations.test.ts b/packages/tools/src/tool-operations.test.ts index cc1dd1e0..d0b46eb1 100644 --- a/packages/tools/src/tool-operations.test.ts +++ b/packages/tools/src/tool-operations.test.ts @@ -341,20 +341,139 @@ describe("memoryForget", () => { }) }) +describe("openai executeToolCall argument parsing", () => { + type ExecutorToolCall = Parameters< + ReturnType + >[0] + + function toolCall(name: string, args: string) { + return { + id: "call_1", + type: "function", + function: { name, arguments: args }, + } as ExecutorToolCall + } + + // getProfile, documentList and memoryForget declare `required: []`, so the model + // is allowed to call them with no arguments. OpenAI serialises that as "". + it.each([ + "", + " ", + ])("runs a zero-argument tool when arguments are %p", async (args) => { + const execute = openAi.createToolCallExecutor(API_KEY, { + namespace: "user_1", + }) + + const result = JSON.parse(await execute(toolCall("getProfile", args))) + + expect(result.success).toBe(true) + expect(sdk.profile).toHaveBeenCalledTimes(1) + expect(sdk.profile.mock.calls[0]?.[0]).toBe("user_1") + }) + + // These parse cleanly, so the JSON guard lets them through to a destructuring + // parameter that rejects — the throw the guard exists to contain. + it.each([ + "null", + "5", + "[]", + '"text"', + ])("rejects non-object arguments %p as a tool result rather than throwing", async (args) => { + const execute = openAi.createToolCallExecutor(API_KEY) + + const result = JSON.parse(await execute(toolCall("getProfile", args))) + + expect(result.success).toBe(false) + expect(result.error).toMatch(/Invalid JSON arguments for getProfile/) + 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.each([ + "constructor", + "toString", + "__proto__", + ])("reports inherited name %p as an unknown function", async (name) => { + const execute = openAi.createToolCallExecutor(API_KEY) + + const result = JSON.parse(await execute(toolCall(name, "{}"))) + + expect(result).toEqual({ + success: false, + error: `Unknown function: ${name}`, + }) + }) + + it("still reports malformed JSON as a tool error", async () => { + const execute = openAi.createToolCallExecutor(API_KEY) + + const result = JSON.parse( + await execute(toolCall("searchMemories", "{not json")), + ) + + expect(result.success).toBe(false) + expect(result.error).toMatch(/Invalid JSON arguments for searchMemories/) + expect(sdk.search).not.toHaveBeenCalled() + }) + + it("still passes well-formed arguments through", async () => { + const execute = openAi.createToolCallExecutor(API_KEY, { + namespace: "user_1", + }) + + const result = JSON.parse( + await execute( + toolCall( + "searchMemories", + JSON.stringify({ informationToGet: "tea", limit: 3 }), + ), + ), + ) + + expect(result.success).toBe(true) + expect(sdk.search).toHaveBeenCalledWith( + "user_1", + expect.objectContaining({ query: "tea", limit: 3 }), + ) + }) +}) + describe("ClaudeMemoryTool", () => { const FILE_PATH = "/memories/prefs.txt" - const FILE_ID = "memories_prefs_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" }, + } }) } @@ -438,7 +557,7 @@ describe("ClaudeMemoryTool", () => { const result = await tool.handleCommand({ command: "insert", path: FILE_PATH, - insert_line: 2, + insert_line: 1, insert_text: "", }) @@ -497,7 +616,7 @@ describe("ClaudeMemoryTool", () => { expect(result.success).toBe(true) expect(sdk.add).toHaveBeenCalledWith( DEFAULT_NS, - expect.objectContaining({ id: "memories_renamed_txt" }), + expect.objectContaining({ id: "cm_memories_s_renamed_d_txt" }), ) expect(sdk.documentsDelete).toHaveBeenCalledWith(DEFAULT_NS, { ids: [FILE_ID], diff --git a/packages/tools/src/tools-shared.test.ts b/packages/tools/src/tools-shared.test.ts index 638749c5..f963b896 100644 --- a/packages/tools/src/tools-shared.test.ts +++ b/packages/tools/src/tools-shared.test.ts @@ -1,4 +1,6 @@ import { describe, expect, it } from "vitest" +import { makeTurnKey } from "./shared/cache" +import { normalizeBaseUrl } from "./shared/context" import { DEFAULT_VALUES, SEARCH_LIMIT_BOUNDS, @@ -120,3 +122,65 @@ describe("deduplicateMemoriesForMode", () => { expect(deduplicated.searchResults).toEqual([]) }) }) + +describe("makeTurnKey", () => { + it("generates predictable turn key for standard inputs", () => { + expect(makeTurnKey("user-123", "thread-456", "full", "hello world")).toBe( + "user-123:thread-456:full:hello world", + ) + }) + + it("normalizes and collapses whitespace in message", () => { + expect( + makeTurnKey("user-123", "thread-456", "full", " hello world \n "), + ).toBe("user-123:thread-456:full:hello world") + }) + + it("handles undefined threadId cleanly", () => { + expect(makeTurnKey("user-123", undefined, "profile", "test")).toBe( + "user-123::profile:test", + ) + }) + + it("escapes colons to prevent cache key collisions between tag and threadId", () => { + const keyA = makeTurnKey("user:123", "456", "profile", "hi") + const keyB = makeTurnKey("user", "123:456", "profile", "hi") + expect(keyA).toBe("user%3A123:456:profile:hi") + expect(keyB).toBe("user:123%3A456:profile:hi") + expect(keyA).not.toBe(keyB) + }) + + it("escapes percent signs to avoid ambiguity with encoded sequences", () => { + const keyA = makeTurnKey("user%3A123", "456", "profile", "hi") + const keyB = makeTurnKey("user:123", "456", "profile", "hi") + expect(keyA).toBe("user%253A123:456:profile:hi") + expect(keyB).toBe("user%3A123:456:profile:hi") + expect(keyA).not.toBe(keyB) + }) +}) + +describe("normalizeBaseUrl", () => { + it("returns default URL when input is missing or empty", () => { + expect(normalizeBaseUrl()).toBe("https://api.supermemory.ai") + expect(normalizeBaseUrl("")).toBe("https://api.supermemory.ai") + expect(normalizeBaseUrl(" ")).toBe("https://api.supermemory.ai") + }) + + it("collapses single and multiple trailing slashes", () => { + expect(normalizeBaseUrl("http://localhost:6768/")).toBe( + "http://localhost:6768", + ) + expect(normalizeBaseUrl("http://localhost:6768///")).toBe( + "http://localhost:6768", + ) + expect(normalizeBaseUrl("https://api.supermemory.ai/")).toBe( + "https://api.supermemory.ai", + ) + }) + + it("trims whitespace around URLs", () => { + expect(normalizeBaseUrl(" http://localhost:6768/ ")).toBe( + "http://localhost:6768", + ) + }) +}) diff --git a/packages/tools/test/claude-memory-commands.test.ts b/packages/tools/test/claude-memory-commands.test.ts new file mode 100644 index 00000000..953a2bc3 --- /dev/null +++ b/packages/tools/test/claude-memory-commands.test.ts @@ -0,0 +1,92 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" + +const sdk = vi.hoisted(() => { + class NotFoundError extends Error { + statusCode = 404 + } + return { + NotFoundError, + get: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + add: vi.fn(), + } +}) + +vi.mock("supermemory", () => { + class Supermemory { + add = sdk.add + list = vi.fn() + documents = { get: sdk.get, update: sdk.update, delete: sdk.delete } + } + return { default: Supermemory, Supermemory, NotFoundError: sdk.NotFoundError } +}) + +import { createClaudeMemoryTool } from "../src/claude-memory" + +function stubFile(path: string, content: string) { + 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", () => { + let tool: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + sdk.update.mockResolvedValue({ id: "doc", status: "queued" }) + sdk.delete.mockResolvedValue({ count: 1 }) + tool = createClaudeMemoryTool("test-api-key") + }) + + it("handles the old_path/new_path shape Claude actually sends", async () => { + stubFile("/memories/draft.txt", "file body") + + const result = await tool.handleCommand({ + command: "rename", + old_path: "/memories/draft.txt", + new_path: "/memories/final.txt", + }) + + expect(result.success).toBe(true) + expect(sdk.update).toHaveBeenCalledWith( + expect.anything(), + expect.any(String), + expect.objectContaining({ content: "file body" }), + ) + }) + + it("still accepts path as the source for older callers", async () => { + stubFile("/memories/draft.txt", "file body") + + const result = await tool.handleCommand({ + command: "rename", + path: "/memories/draft.txt", + new_path: "/memories/final.txt", + }) + + expect(result.success).toBe(true) + }) + + it("validates old_path like any other path", async () => { + const result = await tool.handleCommand({ + command: "rename", + old_path: "/etc/passwd", + new_path: "/memories/final.txt", + }) + + expect(result.success).toBe(false) + expect(result.error).toContain("Invalid path") + }) +})