mirror of
https://github.com/supermemoryai/supermemory.git
synced 2026-10-26 06:04:08 +00:00
fix(tools): close legacy-ID and blank-argument gaps in 3.1.2
- New claude-memory IDs use a cm_ prefix so they can never match a legacy ID, and a write refuses an ID held by another document. - rename overwrites an existing destination (including a legacy ID) instead of leaving a stale copy. - Non-404 lookup failures return an error instead of allowing a write; delete works while content is still processing. - OpenAI executor rejects blank or missing required arguments, and documentDelete requires a documentId. - MemoryCommand.path is required again for every command except rename.
This commit is contained in:
parent
8d12b4bee2
commit
8fdb2d4779
5 changed files with 207 additions and 43 deletions
|
|
@ -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" },
|
||||
|
|
|
|||
|
|
@ -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<MemoryResponse> {
|
||||
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<ReturnType<Supermemory["documents"]["get"]>>
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -332,6 +332,9 @@ export function createDocumentDeleteFunction(
|
|||
}: {
|
||||
documentId: string
|
||||
}): Promise<DocumentDeleteResult> {
|
||||
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))
|
||||
|
|
|
|||
|
|
@ -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],
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue