From 6d8f0a13655caa852dfe35d36d782869e3fd3024 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 31 Jul 2026 16:26:08 -0700 Subject: [PATCH] fix(memory-wiki): protect notes with damaged human markers (#117104) --- .../src/ingest-human-notes.test.ts | 45 +++++ extensions/memory-wiki/src/markdown.test.ts | 108 ++++++++++++ extensions/memory-wiki/src/markdown.ts | 11 +- .../src/source-page-shared.test.ts | 66 ++++++++ .../src/source-sync-malformed-notes.test.ts | 157 ++++++++++++++++++ .../memory-wiki/src/source-sync-state.ts | 10 +- 6 files changed, 387 insertions(+), 10 deletions(-) create mode 100644 extensions/memory-wiki/src/source-sync-malformed-notes.test.ts diff --git a/extensions/memory-wiki/src/ingest-human-notes.test.ts b/extensions/memory-wiki/src/ingest-human-notes.test.ts index 7b07d5dd1561..3882f272a15b 100644 --- a/extensions/memory-wiki/src/ingest-human-notes.test.ts +++ b/extensions/memory-wiki/src/ingest-human-notes.test.ts @@ -39,6 +39,51 @@ describe("ingestMemoryWikiSource human notes", () => { expect(after).toContain(userNote); }); + it.each([ + { + name: "closing", + malformedNotes: "\nHANDWRITTEN NOTE MUST SURVIVE", + missingMarker: /openclaw:human:end/i, + }, + { + name: "opening", + malformedNotes: "HANDWRITTEN NOTE MUST SURVIVE\n", + missingMarker: /openclaw:human:start/i, + }, + ])( + "preserves the source page when handwritten Notes are missing the $name marker", + async ({ malformedNotes, missingMarker }) => { + const rootDir = await createTempDir("memory-wiki-malformed-notes-"); + const inputPath = path.join(rootDir, "roadmap.txt"); + const { config } = await createVault({ rootDir: path.join(rootDir, "vault") }); + + await fs.writeFile(inputPath, "original source content\n", "utf8"); + await ingestMemoryWikiSource({ + config, + inputPath, + nowMs: Date.UTC(2026, 3, 5, 12, 0, 0), + }); + + const pagePath = path.join(config.vault.path, "sources", "roadmap.md"); + const existingPage = (await fs.readFile(pagePath, "utf8")).replace( + "\n", + malformedNotes, + ); + await fs.writeFile(pagePath, existingPage, "utf8"); + await fs.writeFile(inputPath, "updated source content\n", "utf8"); + + await expect( + ingestMemoryWikiSource({ + config, + inputPath, + nowMs: Date.UTC(2026, 3, 6, 12, 0, 0), + }), + ).rejects.toThrow(missingMarker); + + await expect(fs.readFile(pagePath, "utf8")).resolves.toBe(existingPage); + }, + ); + it("preserves notes without corrupting source content that contains human markers", async () => { const rootDir = await createTempDir("memory-wiki-markers-"); const inputPath = path.join(rootDir, "notes.txt"); diff --git a/extensions/memory-wiki/src/markdown.test.ts b/extensions/memory-wiki/src/markdown.test.ts index 7aa0727fde6f..7054030c996f 100644 --- a/extensions/memory-wiki/src/markdown.test.ts +++ b/extensions/memory-wiki/src/markdown.test.ts @@ -4,7 +4,9 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; import { createWikiPageFilename, + extractHumanNotesBlock, parseWikiMarkdown, + preserveHumanNotesBlock, renderWikiMarkdown, scanWikiPageSummary, slugifyWikiSegment, @@ -65,6 +67,112 @@ describe("slugifyWikiSegment", () => { }); }); +describe("human Notes blocks", () => { + const startMarker = ""; + const endMarker = ""; + const rendered = ["# Source", "", "## Notes", startMarker, endMarker, ""].join("\n"); + + it("extracts and preserves complete human Notes blocks", () => { + const existing = rendered.replace( + `${startMarker}\n${endMarker}`, + `${startMarker}\nDurable human annotation\n${endMarker}`, + ); + + expect(extractHumanNotesBlock(existing)).toBe( + `${startMarker}\nDurable human annotation\n${endMarker}`, + ); + expect(preserveHumanNotesBlock(rendered, existing)).toBe(existing); + expect(extractHumanNotesBlock(rendered)).toBeNull(); + }); + + it("leaves pages without human Notes markers unchanged", () => { + const existing = "# Source\n\nGenerated content only.\n"; + + expect(extractHumanNotesBlock(existing)).toBeNull(); + expect(preserveHumanNotesBlock(rendered, existing)).toBe(rendered); + expect(preserveHumanNotesBlock(existing, rendered)).toBe(existing); + }); + + it.each([ + { + name: "closing", + lines: [startMarker, "Durable human annotation"], + missingMarker: endMarker, + }, + { + name: "opening", + lines: ["Durable human annotation", endMarker], + missingMarker: startMarker, + }, + ])( + "rejects a missing $name marker before extracting or replacing Notes", + ({ lines, missingMarker }) => { + const malformed = ["# Source", "", "## Notes", ...lines, ""].join("\n"); + const expectedError = `Memory Wiki human Notes are missing ${missingMarker}; restore the missing marker before updating or removing this page`; + + expect(() => extractHumanNotesBlock(malformed)).toThrow(expectedError); + expect(() => preserveHumanNotesBlock(rendered, malformed)).toThrow(expectedError); + expect(() => preserveHumanNotesBlock(malformed, rendered)).toThrow(expectedError); + }, + ); + + it.each([startMarker, endMarker])( + "ignores a standalone marker inside fenced source content", + (marker) => { + const existing = ["# Source", "", "## Content", "```text", marker, "```", ""].join("\n"); + + expect(extractHumanNotesBlock(existing)).toBeNull(); + expect(preserveHumanNotesBlock(rendered, existing)).toBe(rendered); + }, + ); + + it("preserves marker comments embedded in complete human Notes", () => { + const notes = [ + "Before copied markers", + startMarker, + "Between copied markers", + endMarker, + "After copied markers", + ].join("\n"); + const existing = rendered.replace( + `${startMarker}\n${endMarker}`, + `${startMarker}\n${notes}\n${endMarker}`, + ); + + expect(extractHumanNotesBlock(existing)).toBe(`${startMarker}\n${notes}\n${endMarker}`); + expect(preserveHumanNotesBlock(rendered, existing)).toBe(existing); + }); + + it("ignores source-body marker pairs when extracting and preserving actual Notes", () => { + const sourceWithMarkers = [ + "# Source", + "", + "## Content", + "```text", + startMarker, + "Generated source annotation", + endMarker, + "```", + "", + "## Notes", + startMarker, + "Durable human annotation", + endMarker, + "", + ].join("\n"); + + expect(extractHumanNotesBlock(sourceWithMarkers)).toBe( + `${startMarker}\nDurable human annotation\n${endMarker}`, + ); + expect(preserveHumanNotesBlock(rendered, sourceWithMarkers)).toBe( + rendered.replace( + `${startMarker}\n${endMarker}`, + `${startMarker}\nDurable human annotation\n${endMarker}`, + ), + ); + }); +}); + describe("toWikiPageSummary", () => { it("marks raw and generated source body metadata", () => { const rawSource = toWikiPageSummary({ diff --git a/extensions/memory-wiki/src/markdown.ts b/extensions/memory-wiki/src/markdown.ts index b02d428f5b07..f3a3116cc63d 100644 --- a/extensions/memory-wiki/src/markdown.ts +++ b/extensions/memory-wiki/src/markdown.ts @@ -530,12 +530,15 @@ function afterSourceContentFence(page: string): number { function findNotesHumanBlock(page: string): { start: number; end: number } | null { const searchFrom = afterSourceContentFence(page); const start = page.indexOf(HUMAN_START_MARKER, searchFrom); - if (start === -1) { + const endMarker = page.lastIndexOf(HUMAN_END_MARKER); + if (start === -1 && endMarker < searchFrom) { return null; } - const endMarker = page.lastIndexOf(HUMAN_END_MARKER); - if (endMarker < start) { - return null; + if (start === -1 || endMarker < start) { + const missingMarker = start === -1 ? HUMAN_START_MARKER : HUMAN_END_MARKER; + throw new Error( + `Memory Wiki human Notes are missing ${missingMarker}; restore the missing marker before updating or removing this page`, + ); } return { start, end: endMarker + HUMAN_END_MARKER.length }; } diff --git a/extensions/memory-wiki/src/source-page-shared.test.ts b/extensions/memory-wiki/src/source-page-shared.test.ts index d8dd4186dd2c..0a17156ac482 100644 --- a/extensions/memory-wiki/src/source-page-shared.test.ts +++ b/extensions/memory-wiki/src/source-page-shared.test.ts @@ -220,6 +220,72 @@ describe("writeImportedSourcePage", () => { expect(after).toContain(userNote); }); + it.each([ + { group: "bridge" as const, missingMarker: "opening" as const }, + { group: "bridge" as const, missingMarker: "closing" as const }, + { group: "unsafe-local" as const, missingMarker: "opening" as const }, + { group: "unsafe-local" as const, missingMarker: "closing" as const }, + ])( + "preserves a $group page and tracked state when its human Notes $missingMarker marker is missing", + async ({ group, missingMarker }) => { + const sourcePath = path.join(suiteRoot, "imported-malformed.txt"); + const pagePath = "sources/imported-malformed.md"; + const syncKey = `${group}:imported-malformed`; + const state: Parameters[0]["state"] = { + entries: {}, + version: 1, + }; + const firstSource = "first imported body"; + + await fs.writeFile(sourcePath, firstSource, "utf8"); + await writeImportedSourcePage({ + vaultRoot: suiteRoot, + syncKey, + sourcePath, + sourceUpdatedAtMs: Date.UTC(2026, 4, 1), + sourceSize: Buffer.byteLength(firstSource), + renderFingerprint: "fp-1", + pagePath, + group, + state, + buildRendered: buildSourcePage, + }); + + const absPage = path.join(suiteRoot, pagePath); + const userNote = `DURABLE ${group.toUpperCase()} ANNOTATION`; + const malformedNotes = + missingMarker === "opening" + ? `${userNote}\n` + : `\n${userNote}`; + const existing = (await fs.readFile(absPage, "utf8")).replace( + "\n", + malformedNotes, + ); + await fs.writeFile(absPage, existing, "utf8"); + const previousState = structuredClone(state); + const secondSource = "second imported body must not overwrite human Notes"; + await fs.writeFile(sourcePath, secondSource, "utf8"); + + await expect( + writeImportedSourcePage({ + vaultRoot: suiteRoot, + syncKey, + sourcePath, + sourceUpdatedAtMs: Date.UTC(2026, 4, 2), + sourceSize: Buffer.byteLength(secondSource), + renderFingerprint: "fp-2", + pagePath, + group, + state, + buildRendered: buildSourcePage, + }), + ).rejects.toThrow(/human Notes.*missing|missing.*human Notes|openclaw:human:(start|end)/i); + + await expect(fs.readFile(absPage, "utf8")).resolves.toBe(existing); + expect(state).toEqual(previousState); + }, + ); + it("preserves CRLF human notes without copying marker comments from existing imported content", async () => { const sourcePath = path.join(suiteRoot, "imported-crlf.txt"); const pagePath = "sources/imported-crlf.md"; diff --git a/extensions/memory-wiki/src/source-sync-malformed-notes.test.ts b/extensions/memory-wiki/src/source-sync-malformed-notes.test.ts new file mode 100644 index 000000000000..c16a0b9449ad --- /dev/null +++ b/extensions/memory-wiki/src/source-sync-malformed-notes.test.ts @@ -0,0 +1,157 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import type { OpenKeyedStoreOptions } from "openclaw/plugin-sdk/plugin-state-runtime"; +import { + createPluginStateKeyedStoreForTests, + resetPluginStateStoreForTests, +} from "openclaw/plugin-sdk/plugin-state-test-runtime"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + createMemoryWikiSourceSyncStateStore, + pruneImportedSourceEntries, + readMemoryWikiSourceSyncState, + setImportedSourceEntry, + writeMemoryWikiSourceSyncState, + type MemoryWikiImportedSourceGroup, +} from "./source-sync-state.js"; + +const tempDirs: string[] = []; + +async function makeTempDir(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "memory-wiki-malformed-notes-")); + tempDirs.push(dir); + return dir; +} + +function createImportedSourceState(pagePath: string, group: MemoryWikiImportedSourceGroup) { + return { + version: 1 as const, + entries: { + "sync-key": { + group, + pagePath, + sourcePath: "/tmp/source.md", + sourceUpdatedAtMs: 0, + sourceSize: 0, + renderFingerprint: "fp", + }, + }, + }; +} + +describe("memory wiki source sync malformed human Notes", () => { + beforeEach(() => { + resetPluginStateStoreForTests(); + }); + + afterEach(async () => { + resetPluginStateStoreForTests(); + await Promise.all( + tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })), + ); + }); + + it.each([ + { group: "bridge" as const, missingMarker: "opening" as const }, + { group: "bridge" as const, missingMarker: "closing" as const }, + { group: "unsafe-local" as const, missingMarker: "opening" as const }, + { group: "unsafe-local" as const, missingMarker: "closing" as const }, + ])( + "preserves $group pages and persisted state when the $missingMarker marker is missing", + async ({ group, missingMarker }) => { + const stateDir = await makeTempDir(); + const vaultRoot = path.join(stateDir, "vault"); + const pagePath = `sources/${group}-missing-${missingMarker}.md`; + const pageAbsPath = path.join(vaultRoot, pagePath); + await fs.mkdir(path.dirname(pageAbsPath), { recursive: true }); + const pageContent = [ + group === "bridge" ? "# Memory Bridge (test)" : "# Unsafe Local Import: test", + "## Content", + "```", + "generated content", + "```", + "## Notes", + ...(missingMarker === "opening" ? [] : [""]), + "human annotations must survive malformed markers", + ...(missingMarker === "closing" ? [] : [""]), + "", + ].join("\n"); + await fs.writeFile(pageAbsPath, pageContent, "utf8"); + + const env = { ...process.env, OPENCLAW_STATE_DIR: stateDir }; + const store = createMemoryWikiSourceSyncStateStore((options: OpenKeyedStoreOptions) => + createPluginStateKeyedStoreForTests("memory-wiki", { ...options, env }), + ); + const initialState = createImportedSourceState(pagePath, group); + await writeMemoryWikiSourceSyncState(vaultRoot, initialState, store); + const state = await readMemoryWikiSourceSyncState(vaultRoot, store); + const entry = state.entries["sync-key"]!; + const updatedEntry = { ...entry, sourceSize: entry.sourceSize + 1 }; + setImportedSourceEntry({ state, syncKey: "sync-key", entry: updatedEntry }); + const expectedMissingMarker = + missingMarker === "opening" + ? "" + : ""; + + await expect( + pruneImportedSourceEntries({ + vaultRoot, + group, + activeKeys: new Set(), + state, + }), + ).rejects.toThrow(expectedMissingMarker); + + expect(state.entries["sync-key"]).toEqual(updatedEntry); + await expect(readMemoryWikiSourceSyncState(vaultRoot, store)).resolves.toEqual(initialState); + await expect(fs.readFile(pageAbsPath, "utf8")).resolves.toBe(pageContent); + await expect(fs.access(path.join(vaultRoot, ".salvage"))).rejects.toMatchObject({ + code: "ENOENT", + }); + + await writeMemoryWikiSourceSyncState(vaultRoot, state, store); + await expect(readMemoryWikiSourceSyncState(vaultRoot, store)).resolves.toMatchObject({ + entries: { "sync-key": updatedEntry }, + }); + }, + ); + + it("rejects pruning an oversized source page with an unclosed human Notes marker", async () => { + const vaultRoot = await makeTempDir(); + const pagePath = "sources/oversized-missing-closing-marker.md"; + const pageAbsPath = path.join(vaultRoot, pagePath); + await fs.mkdir(path.dirname(pageAbsPath), { recursive: true }); + await fs.writeFile( + pageAbsPath, + [ + "# Memory Bridge (test)", + "## Content", + "```", + "x".repeat(16 * 1024 * 1024), + "```", + "## Notes", + "", + "oversized annotations must survive malformed markers", + "", + ].join("\n"), + "utf8", + ); + const state = createImportedSourceState(pagePath, "bridge"); + + await expect( + pruneImportedSourceEntries({ + vaultRoot, + group: "bridge", + activeKeys: new Set(), + state, + }), + ).rejects.toThrow(""); + + expect(state.entries["sync-key"]).toBeDefined(); + await expect(fs.stat(pageAbsPath)).resolves.toSatisfy((stat) => stat.isFile()); + await expect(fs.access(path.join(vaultRoot, ".salvage"))).rejects.toMatchObject({ + code: "ENOENT", + }); + }); +}); diff --git a/extensions/memory-wiki/src/source-sync-state.ts b/extensions/memory-wiki/src/source-sync-state.ts index 49c511f99463..5a4bda76362f 100644 --- a/extensions/memory-wiki/src/source-sync-state.ts +++ b/extensions/memory-wiki/src/source-sync-state.ts @@ -454,17 +454,15 @@ export async function pruneImportedSourceEntries(params: { } // Recover durable Notes before removing an imported source page. The root // handle applies containment and no-follow checks to each operation. - let notesBlock: string | null = null; - let pageAlreadyRemoved = false; + let pageContent: string | undefined; try { - const pageContent = await readImportedSourcePageForNotes(vault, entry.pagePath); - notesBlock = extractHumanNotesBlock(pageContent); + pageContent = await readImportedSourcePageForNotes(vault, entry.pagePath); } catch (error) { if (!(error instanceof FsSafeError && error.code === "not-found")) { continue; } - pageAlreadyRemoved = true; } + const notesBlock = pageContent === undefined ? null : extractHumanNotesBlock(pageContent); if (notesBlock) { const salvageStem = entry.pagePath.replace(/\//g, "_"); const contentHash = createHash("sha256").update(notesBlock).digest("hex").slice(0, 16); @@ -501,7 +499,7 @@ export async function pruneImportedSourceEntries(params: { continue; } } - if (!pageAlreadyRemoved) { + if (pageContent !== undefined) { try { await vault.remove(entry.pagePath); } catch (error) {