From 58fa23b4a2f24e2eb254dec9517c5c387ef4fc11 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 7 May 2026 04:16:04 +0100 Subject: [PATCH] test: align fs-safe dependency expectations --- src/infra/archive.test.ts | 2 +- src/infra/fs-safe.test.ts | 4 +- src/infra/json-file.test.ts | 6 +- src/infra/json-files.test.ts | 4 +- src/media/store.test.ts | 143 +++++++++++------- src/plugin-sdk/browser-maintenance.test.ts | 161 ++++++++++++--------- 6 files changed, 188 insertions(+), 132 deletions(-) diff --git a/src/infra/archive.test.ts b/src/infra/archive.test.ts index bc39fad97b1..eea7141a702 100644 --- a/src/infra/archive.test.ts +++ b/src/infra/archive.test.ts @@ -240,7 +240,7 @@ describe("archive utils", () => { } catch (error) { rejected = true; expect(error).toMatchObject({ - code: "destination-symlink-traversal", + code: expect.stringMatching(/destination-symlink-traversal|not-file/), } satisfies Partial); } diff --git a/src/infra/fs-safe.test.ts b/src/infra/fs-safe.test.ts index d02d2534fe8..91cf1d75a89 100644 --- a/src/infra/fs-safe.test.ts +++ b/src/infra/fs-safe.test.ts @@ -38,7 +38,9 @@ async function runWriteOpenRace(params: { await params.runWrite(); } catch (err) { expect(err).toMatchObject({ - code: expect.stringMatching(/outside-workspace|path-mismatch|path-alias|invalid-path/), + code: expect.stringMatching( + /outside-workspace|path-mismatch|path-alias|invalid-path|not-file/, + ), }); } }, diff --git a/src/infra/json-file.test.ts b/src/infra/json-file.test.ts index 7d2de812987..b6c7cc8a670 100644 --- a/src/infra/json-file.test.ts +++ b/src/infra/json-file.test.ts @@ -175,10 +175,9 @@ describe("json-file helpers", () => { }, ); - it("falls back to copy when rename-based overwrite fails", async () => { + it("preserves payload when rename-based overwrite reports EPERM", async () => { await withJsonPath(({ root, pathname }) => { writeExistingJson(pathname); - const copySpy = vi.spyOn(fs, "copyFileSync"); const renameSpy = vi.spyOn(fs, "renameSync").mockImplementationOnce(() => { const err = new Error("EPERM") as NodeJS.ErrnoException; err.code = "EPERM"; @@ -187,8 +186,7 @@ describe("json-file helpers", () => { saveJsonFile(pathname, SAVED_PAYLOAD); - expect(renameSpy).toHaveBeenCalledOnce(); - expect(copySpy).toHaveBeenCalledOnce(); + expect(renameSpy).toHaveBeenCalled(); expect(loadJsonFile(pathname)).toEqual(SAVED_PAYLOAD); expect(fs.readdirSync(root)).toEqual(["config.json"]); }); diff --git a/src/infra/json-files.test.ts b/src/infra/json-files.test.ts index d0c3a29e09b..3f65861a04f 100644 --- a/src/infra/json-files.test.ts +++ b/src/infra/json-files.test.ts @@ -96,7 +96,7 @@ describe("json file helpers", () => { }); }); - it("falls back to copy-on-replace for Windows rename EPERM", async () => { + it("preserves text when Windows rename reports EPERM", async () => { await withTempDir({ prefix: "openclaw-json-files-" }, async (base) => { const filePath = path.join(base, "state.json"); await fs.writeFile(filePath, "old", "utf8"); @@ -104,12 +104,10 @@ describe("json file helpers", () => { Object.defineProperty(process, "platform", { value: "win32", configurable: true }); const renameError = Object.assign(new Error("EPERM"), { code: "EPERM" }); const renameSpy = vi.spyOn(fs, "rename").mockRejectedValueOnce(renameError); - const copySpy = vi.spyOn(fs, "copyFile"); await writeTextAtomic(filePath, "new"); expect(renameSpy).toHaveBeenCalledOnce(); - expect(copySpy).toHaveBeenCalledOnce(); await expect(fs.readFile(filePath, "utf8")).resolves.toBe("new"); }); }); diff --git a/src/media/store.test.ts b/src/media/store.test.ts index 46caab52d12..a1860ae311b 100644 --- a/src/media/store.test.ts +++ b/src/media/store.test.ts @@ -52,30 +52,97 @@ describe("media store", () => { segment: string; run: (store: typeof import("./store.js"), home: string) => Promise<{ path: string }>; }) { - await withTempStore(async (store, home) => { - const originalWriteFile = fs.writeFile.bind(fs); - let injectedEnoent = false; - vi.spyOn(fs, "writeFile").mockImplementation(async (...args) => { - const [filePath] = args; - if ( - !injectedEnoent && - typeof filePath === "string" && - filePath.includes(`${path.sep}${params.segment}${path.sep}`) - ) { - injectedEnoent = true; - await fs.rm(path.dirname(filePath), { recursive: true, force: true }); - const err = new Error("missing dir") as NodeJS.ErrnoException; - err.code = "ENOENT"; - throw err; - } - return await originalWriteFile(...args); - }); - - const saved = await params.run(store, home); - const savedStat = await fs.stat(saved.path); - expect(injectedEnoent).toBe(true); - expect(savedStat.isFile()).toBe(true); + const mockKey = `./store.js?scope=retry-pruned-write-${params.segment}-${Date.now()}-${Math.random().toString(36).slice(2)}`; + let injectedEnoent = false; + vi.doMock("../infra/file-store.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fileStore: (options: Parameters[0]) => { + const actualStore = actual.fileStore(options); + return { + ...actualStore, + write: async (...args: Parameters) => { + const [relativePath] = args; + if (!injectedEnoent && relativePath.includes(`${params.segment}${path.sep}`)) { + injectedEnoent = true; + await fs.rm(path.dirname(actualStore.path(relativePath)), { + recursive: true, + force: true, + }); + const err = new Error("missing dir") as NodeJS.ErrnoException; + err.code = "ENOENT"; + throw err; + } + return await actualStore.write(...args); + }, + }; + }, + }; }); + + try { + const storeWithMock = await importFreshModule( + import.meta.url, + mockKey, + ); + await withTempStore(async (_store, home) => { + const saved = await params.run(storeWithMock, home); + const savedStat = await fs.stat(saved.path); + expect(injectedEnoent).toBe(true); + expect(savedStat.isFile()).toBe(true); + }); + } finally { + vi.doUnmock("../infra/file-store.js"); + } + } + + async function expectFailedBufferWriteCase() { + const mockKey = `./store.js?scope=failed-buffer-write-${Date.now()}-${Math.random().toString(36).slice(2)}`; + const attemptedRelPaths: string[] = []; + vi.doMock("../infra/file-store.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fileStore: (options: Parameters[0]) => { + const actualStore = actual.fileStore(options); + return { + ...actualStore, + write: async (...args: Parameters) => { + const [relativePath] = args; + if (relativePath.includes(`failed-buffer${path.sep}`)) { + attemptedRelPaths.push(relativePath); + const err = new Error("no space left on device") as NodeJS.ErrnoException; + err.code = "ENOSPC"; + throw err; + } + return await actualStore.write(...args); + }, + }; + }, + }; + }); + + try { + const storeWithMock = await importFreshModule( + import.meta.url, + mockKey, + ); + await withTempStore(async (_store) => { + const mediaDir = await storeWithMock.ensureMediaDir(); + await expect( + storeWithMock.saveMediaBuffer(Buffer.from("voice"), "audio/ogg", "failed-buffer"), + ).rejects.toMatchObject({ code: "ENOSPC" }); + + const failedDir = path.join(mediaDir, "failed-buffer"); + const entries = await fs.readdir(failedDir).catch(() => []); + expect(attemptedRelPaths).toHaveLength(1); + expect(path.basename(attemptedRelPaths[0] ?? "")).toMatch(/^[^/\\]+\.ogg$/); + expect(entries).toEqual([]); + }); + } finally { + vi.doUnmock("../infra/file-store.js"); + } } async function expectSavedOriginalFilenameCase(params: { @@ -310,35 +377,7 @@ describe("media store", () => { { name: "does not leave final media artifacts when buffer writes fail", run: async () => { - await withTempStore(async (store) => { - const mediaDir = await store.ensureMediaDir(); - const originalWriteFile = fs.writeFile.bind(fs); - const attemptedPaths: string[] = []; - vi.spyOn(fs, "writeFile").mockImplementation(async (...args) => { - const [filePath] = args; - if ( - typeof filePath === "string" && - filePath.includes(`${path.sep}failed-buffer${path.sep}`) - ) { - attemptedPaths.push(filePath); - await originalWriteFile(filePath, Buffer.alloc(0), args[2]); - const err = new Error("no space left on device") as NodeJS.ErrnoException; - err.code = "ENOSPC"; - throw err; - } - return await originalWriteFile(...args); - }); - - await expect( - store.saveMediaBuffer(Buffer.from("voice"), "audio/ogg", "failed-buffer"), - ).rejects.toMatchObject({ code: "ENOSPC" }); - - const failedDir = path.join(mediaDir, "failed-buffer"); - const entries = await fs.readdir(failedDir).catch(() => []); - expect(attemptedPaths).toHaveLength(1); - expect(path.basename(attemptedPaths[0] ?? "")).toMatch(/^\..+\.tmp$/); - expect(entries).toEqual([]); - }); + await expectFailedBufferWriteCase(); }, }, { diff --git a/src/plugin-sdk/browser-maintenance.test.ts b/src/plugin-sdk/browser-maintenance.test.ts index b19b25d4965..9cd6c7b81b3 100644 --- a/src/plugin-sdk/browser-maintenance.test.ts +++ b/src/plugin-sdk/browser-maintenance.test.ts @@ -1,10 +1,16 @@ import fs from "node:fs"; import os from "node:os"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const closeTrackedBrowserTabsForSessionsImpl = vi.hoisted(() => vi.fn()); const loadBundledPluginPublicSurfaceModuleSync = vi.hoisted(() => vi.fn()); const runExec = vi.hoisted(() => vi.fn()); +const realMkdirSync = fs.mkdirSync.bind(fs); +const realMkdtempSync = fs.mkdtempSync.bind(fs); +const realRmSync = fs.rmSync.bind(fs); +const realWriteFileSync = fs.writeFileSync.bind(fs); +const realRealpathSyncNative = fs.realpathSync.native.bind(fs.realpathSync); vi.mock("./facade-loader.js", () => ({ loadBundledPluginPublicSurfaceModuleSync, @@ -19,29 +25,53 @@ function mockTrashContainer(...suffixes: string[]) { return vi.spyOn(fs, "mkdtempSync").mockImplementation((prefix) => { const suffix = suffixes[call] ?? "secure"; call += 1; - return `${prefix}${suffix}`; + const container = `${prefix}${suffix}`; + realMkdirSync(container, { recursive: true }); + return container; }); } describe("browser maintenance", () => { + let testRoot = ""; + let homeDir = ""; + let tmpDir = ""; + beforeEach(() => { vi.restoreAllMocks(); + testRoot = realRealpathSyncNative( + realMkdtempSync(path.join(os.tmpdir(), "openclaw-browser-maintenance-")), + ); + homeDir = path.join(testRoot, "home", "test"); + tmpDir = path.join(testRoot, "tmp"); + realMkdirSync(path.join(homeDir, ".Trash"), { recursive: true, mode: 0o700 }); + realMkdirSync(tmpDir, { recursive: true, mode: 0o700 }); closeTrackedBrowserTabsForSessionsImpl.mockReset(); loadBundledPluginPublicSurfaceModuleSync.mockReset(); runExec.mockReset(); vi.spyOn(Date, "now").mockReturnValue(123); - vi.spyOn(os, "homedir").mockReturnValue("/home/test"); - vi.spyOn(os, "tmpdir").mockReturnValue("/tmp"); - vi.spyOn(fs, "lstatSync").mockReturnValue({ - isDirectory: () => true, - isSymbolicLink: () => false, - } as fs.Stats); - vi.spyOn(fs.realpathSync, "native").mockImplementation((candidate) => String(candidate)); + vi.spyOn(os, "homedir").mockReturnValue(homeDir); + vi.spyOn(os, "tmpdir").mockReturnValue(tmpDir); + vi.spyOn(fs.realpathSync, "native").mockImplementation((candidate) => + realRealpathSyncNative(candidate), + ); loadBundledPluginPublicSurfaceModuleSync.mockReturnValue({ closeTrackedBrowserTabsForSessions: closeTrackedBrowserTabsForSessionsImpl, }); }); + afterEach(() => { + vi.restoreAllMocks(); + if (testRoot) { + realRmSync(testRoot, { recursive: true, force: true }); + } + }); + + function writeTrashTarget(name = "demo"): string { + const target = path.join(tmpDir, name); + realWriteFileSync(target, "demo"); + return target; + } + it("skips browser cleanup when no session keys are provided", async () => { const { closeTrackedBrowserTabsForSessions } = await import("./browser-maintenance.js"); @@ -74,46 +104,47 @@ describe("browser maintenance", () => { const rmSync = vi.spyOn(fs, "rmSync"); const { movePathToTrash } = await import("./browser-maintenance.js"); + const target = writeTrashTarget(); + const expected = path.join(homeDir, ".Trash", "demo-123-secure", "demo"); - await expect(movePathToTrash("/tmp/demo")).resolves.toBe( - "/home/test/.Trash/demo-123-secure/demo", - ); + await expect(movePathToTrash(target)).resolves.toBe(expected); expect(runExec).not.toHaveBeenCalled(); - expect(mkdirSync).toHaveBeenCalledWith("/home/test/.Trash", { + expect(mkdirSync).toHaveBeenCalledWith(path.join(homeDir, ".Trash"), { recursive: true, mode: 0o700, }); - expect(mkdtempSync).toHaveBeenCalledWith("/home/test/.Trash/demo-123-"); - expect(renameSync).toHaveBeenCalledWith("/tmp/demo", "/home/test/.Trash/demo-123-secure/demo"); + expect(mkdtempSync).toHaveBeenCalledWith(path.join(homeDir, ".Trash", "demo-123-")); + expect(renameSync).toHaveBeenCalledWith(target, expected); expect(cpSync).not.toHaveBeenCalled(); expect(rmSync).not.toHaveBeenCalled(); }); it("uses the resolved trash directory for reserved destinations", async () => { vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); + const resolvedHomeDir = path.join(testRoot, "real", "home", "test"); + const resolvedTrashDir = path.join(resolvedHomeDir, ".Trash"); + realMkdirSync(path.join(homeDir, ".Trash"), { recursive: true, mode: 0o700 }); + realMkdirSync(resolvedTrashDir, { recursive: true, mode: 0o700 }); vi.spyOn(fs.realpathSync, "native").mockImplementation((candidate) => { const value = String(candidate); - if (value === "/home/test") { - return "/real/home/test"; + if (value === homeDir) { + return resolvedHomeDir; } - if (value === "/home/test/.Trash") { - return "/real/home/test/.Trash"; + if (value === path.join(homeDir, ".Trash")) { + return resolvedTrashDir; } - return value; + return realRealpathSyncNative(candidate); }); const mkdtempSync = mockTrashContainer("secure"); const renameSync = vi.spyOn(fs, "renameSync").mockImplementation(() => undefined); const { movePathToTrash } = await import("./browser-maintenance.js"); + const target = writeTrashTarget(); + const expected = path.join(resolvedTrashDir, "demo-123-secure", "demo"); - await expect(movePathToTrash("/tmp/demo")).resolves.toBe( - "/real/home/test/.Trash/demo-123-secure/demo", - ); - expect(mkdtempSync).toHaveBeenCalledWith("/real/home/test/.Trash/demo-123-"); - expect(renameSync).toHaveBeenCalledWith( - "/tmp/demo", - "/real/home/test/.Trash/demo-123-secure/demo", - ); + await expect(movePathToTrash(target)).resolves.toBe(expected); + expect(mkdtempSync).toHaveBeenCalledWith(path.join(resolvedTrashDir, "demo-123-")); + expect(renameSync).toHaveBeenCalledWith(target, expected); }); it("refuses to trash filesystem roots", async () => { @@ -124,8 +155,12 @@ describe("browser maintenance", () => { it("refuses to trash paths outside allowed roots", async () => { const { movePathToTrash } = await import("./browser-maintenance.js"); + const outsideDir = path.join(testRoot, "outside"); + realMkdirSync(outsideDir, { recursive: true }); + const outsidePath = path.join(outsideDir, "openclaw-demo"); + realWriteFileSync(outsidePath, "outside"); - await expect(movePathToTrash("/etc/openclaw-demo")).rejects.toThrow( + await expect(movePathToTrash(outsidePath)).rejects.toThrow( "Refusing to trash path outside allowed roots", ); }); @@ -139,7 +174,7 @@ describe("browser maintenance", () => { const { movePathToTrash } = await import("./browser-maintenance.js"); - await expect(movePathToTrash("/tmp/demo")).rejects.toThrow( + await expect(movePathToTrash(writeTrashTarget())).rejects.toThrow( "Refusing to use non-directory/symlink trash directory", ); }); @@ -155,16 +190,16 @@ describe("browser maintenance", () => { const rmSync = vi.spyOn(fs, "rmSync").mockImplementation(() => undefined); const { movePathToTrash } = await import("./browser-maintenance.js"); + const target = writeTrashTarget(); + const expected = path.join(homeDir, ".Trash", "demo-123-secure", "demo"); - await expect(movePathToTrash("/tmp/demo")).resolves.toBe( - "/home/test/.Trash/demo-123-secure/demo", - ); - expect(cpSync).toHaveBeenCalledWith("/tmp/demo", "/home/test/.Trash/demo-123-secure/demo", { + await expect(movePathToTrash(target)).resolves.toBe(expected); + expect(cpSync).toHaveBeenCalledWith(target, expected, { recursive: true, force: false, errorOnExist: true, }); - expect(rmSync).toHaveBeenCalledWith("/tmp/demo", { recursive: true, force: false }); + expect(rmSync).toHaveBeenCalledWith(target, { recursive: true, force: false }); }); it("retries copy fallback when the copy destination is created concurrently", async () => { @@ -186,30 +221,21 @@ describe("browser maintenance", () => { const rmSync = vi.spyOn(fs, "rmSync").mockImplementation(() => undefined); const { movePathToTrash } = await import("./browser-maintenance.js"); + const target = writeTrashTarget(); + const first = path.join(homeDir, ".Trash", "demo-123-first", "demo"); + const second = path.join(homeDir, ".Trash", "demo-123-second", "demo"); - await expect(movePathToTrash("/tmp/demo")).resolves.toBe( - "/home/test/.Trash/demo-123-second/demo", - ); - expect(cpSync).toHaveBeenNthCalledWith( - 1, - "/tmp/demo", - "/home/test/.Trash/demo-123-first/demo", - { - recursive: true, - force: false, - errorOnExist: true, - }, - ); - expect(cpSync).toHaveBeenNthCalledWith( - 2, - "/tmp/demo", - "/home/test/.Trash/demo-123-second/demo", - { - recursive: true, - force: false, - errorOnExist: true, - }, - ); + await expect(movePathToTrash(target)).resolves.toBe(second); + expect(cpSync).toHaveBeenNthCalledWith(1, target, first, { + recursive: true, + force: false, + errorOnExist: true, + }); + expect(cpSync).toHaveBeenNthCalledWith(2, target, second, { + recursive: true, + force: false, + errorOnExist: true, + }); expect(rmSync).toHaveBeenCalledTimes(1); expect(Date.now).toHaveBeenCalledTimes(1); }); @@ -226,20 +252,13 @@ describe("browser maintenance", () => { .mockImplementation(() => undefined); const { movePathToTrash } = await import("./browser-maintenance.js"); + const target = writeTrashTarget(); + const first = path.join(homeDir, ".Trash", "demo-123-first", "demo"); + const second = path.join(homeDir, ".Trash", "demo-123-second", "demo"); - await expect(movePathToTrash("/tmp/demo")).resolves.toBe( - "/home/test/.Trash/demo-123-second/demo", - ); - expect(renameSync).toHaveBeenNthCalledWith( - 1, - "/tmp/demo", - "/home/test/.Trash/demo-123-first/demo", - ); - expect(renameSync).toHaveBeenNthCalledWith( - 2, - "/tmp/demo", - "/home/test/.Trash/demo-123-second/demo", - ); + await expect(movePathToTrash(target)).resolves.toBe(second); + expect(renameSync).toHaveBeenNthCalledWith(1, target, first); + expect(renameSync).toHaveBeenNthCalledWith(2, target, second); expect(Date.now).toHaveBeenCalledTimes(1); }); });