diff --git a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts index 5a0a895c47d..fdc2a5dc1ab 100644 --- a/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts +++ b/src/browser/pw-tools-core.waits-next-download-saves-it.test.ts @@ -78,6 +78,21 @@ describe("pw-tools-core", () => { }; } + async function expectAtomicDownloadSave(params: { + saveAs: ReturnType; + targetPath: string; + tempDir: string; + content: string; + }) { + const savedPath = params.saveAs.mock.calls[0]?.[0]; + expect(typeof savedPath).toBe("string"); + expect(savedPath).not.toBe(params.targetPath); + expect(path.dirname(String(savedPath))).toBe(params.tempDir); + expect(path.basename(String(savedPath))).toContain(".openclaw-output-"); + expect(path.basename(String(savedPath))).toContain(".part"); + expect(await fs.readFile(params.targetPath, "utf8")).toBe(params.content); + } + it("waits for the next download and atomically finalizes explicit output paths", async () => { await withTempDir(async (tempDir) => { const harness = createDownloadEventHarness(); @@ -104,13 +119,7 @@ describe("pw-tools-core", () => { harness.trigger(download); const res = await p; - const savedPath = saveAs.mock.calls[0]?.[0]; - expect(typeof savedPath).toBe("string"); - expect(savedPath).not.toBe(targetPath); - expect(path.dirname(String(savedPath))).toBe(tempDir); - expect(path.basename(String(savedPath))).toContain(".openclaw-output-"); - expect(path.basename(String(savedPath))).toContain(".part"); - expect(await fs.readFile(targetPath, "utf8")).toBe("file-content"); + await expectAtomicDownloadSave({ saveAs, targetPath, tempDir, content: "file-content" }); expect(res.path).toBe(targetPath); }); }); @@ -146,13 +155,7 @@ describe("pw-tools-core", () => { harness.trigger(download); const res = await p; - const savedPath = saveAs.mock.calls[0]?.[0]; - expect(typeof savedPath).toBe("string"); - expect(savedPath).not.toBe(targetPath); - expect(path.dirname(String(savedPath))).toBe(tempDir); - expect(path.basename(String(savedPath))).toContain(".openclaw-output-"); - expect(path.basename(String(savedPath))).toContain(".part"); - expect(await fs.readFile(targetPath, "utf8")).toBe("report-content"); + await expectAtomicDownloadSave({ saveAs, targetPath, tempDir, content: "report-content" }); expect(res.path).toBe(targetPath); }); }); diff --git a/src/browser/server.auth-fail-closed.test.ts b/src/browser/server.auth-fail-closed.test.ts index 67228c5ad4a..451b6196473 100644 --- a/src/browser/server.auth-fail-closed.test.ts +++ b/src/browser/server.auth-fail-closed.test.ts @@ -1,5 +1,5 @@ -import { createServer, type AddressInfo } from "node:net"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getFreePort } from "./test-port.js"; const mocks = vi.hoisted(() => ({ controlPort: 0, @@ -12,12 +12,13 @@ const mocks = vi.hoisted(() => ({ vi.mock("../config/config.js", async (importOriginal) => { const actual = await importOriginal(); + const browserConfig = { + enabled: true, + }; return { ...actual, loadConfig: () => ({ - browser: { - enabled: true, - }, + browser: browserConfig, }), }; }); @@ -58,17 +59,6 @@ vi.mock("./pw-ai-state.js", () => ({ const { startBrowserControlServerFromConfig, stopBrowserControlServer } = await import("./server.js"); -async function getFreePort(): Promise { - const probe = createServer(); - await new Promise((resolve, reject) => { - probe.once("error", reject); - probe.listen(0, "127.0.0.1", () => resolve()); - }); - const addr = probe.address() as AddressInfo; - await new Promise((resolve) => probe.close(() => resolve())); - return addr.port; -} - describe("browser control auth bootstrap failures", () => { beforeEach(async () => { mocks.controlPort = await getFreePort(); diff --git a/src/browser/server.evaluate-disabled-does-not-block-storage.test.ts b/src/browser/server.evaluate-disabled-does-not-block-storage.test.ts index 03b10299dbd..22c027b2d4c 100644 --- a/src/browser/server.evaluate-disabled-does-not-block-storage.test.ts +++ b/src/browser/server.evaluate-disabled-does-not-block-storage.test.ts @@ -1,6 +1,6 @@ -import { createServer, type AddressInfo } from "node:net"; import { fetch as realFetch } from "undici"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getFreePort } from "./test-port.js"; let testPort = 0; let prevGatewayPort: string | undefined; @@ -68,17 +68,6 @@ vi.mock("./server-context.js", async (importOriginal) => { const { startBrowserControlServerFromConfig, stopBrowserControlServer } = await import("./server.js"); -async function getFreePort(): Promise { - const probe = createServer(); - await new Promise((resolve, reject) => { - probe.once("error", reject); - probe.listen(0, "127.0.0.1", () => resolve()); - }); - const addr = probe.address() as AddressInfo; - await new Promise((resolve) => probe.close(() => resolve())); - return addr.port; -} - describe("browser control evaluate gating", () => { beforeEach(async () => { testPort = await getFreePort(); diff --git a/src/memory/index.test.ts b/src/memory/index.test.ts index 861862d4f5c..4da434c55de 100644 --- a/src/memory/index.test.ts +++ b/src/memory/index.test.ts @@ -127,6 +127,17 @@ describe("memory index", () => { }; } + function requireManager( + result: Awaited>, + missingMessage = "manager missing", + ): MemoryIndexManager { + expect(result.manager).not.toBeNull(); + if (!result.manager) { + throw new Error(missingMessage); + } + return result.manager as MemoryIndexManager; + } + async function getPersistentManager(cfg: TestCfg): Promise { const storePath = cfg.agents?.defaults?.memorySearch?.store?.path; if (!storePath) { @@ -139,17 +150,26 @@ describe("memory index", () => { } const result = await getMemorySearchManager({ cfg, agentId: "main" }); - expect(result.manager).not.toBeNull(); - if (!result.manager) { - throw new Error("manager missing"); - } - const manager = result.manager as MemoryIndexManager; + const manager = requireManager(result); managersByStorePath.set(storePath, manager); managersForCleanup.add(manager); resetManagerForTest(manager); return manager; } + async function expectHybridKeywordSearchFindsMemory(cfg: TestCfg) { + const manager = await getPersistentManager(cfg); + const status = manager.status(); + if (!status.fts?.available) { + return; + } + + await manager.sync({ reason: "test" }); + const results = await manager.search("zebra"); + expect(results.length).toBeGreaterThan(0); + expect(results[0]?.path).toContain("memory/2026-01-12.md"); + } + it("indexes memory files and searches", async () => { const cfg = createCfg({ storePath: indexMainPath, @@ -178,26 +198,19 @@ describe("memory index", () => { const cfg = createCfg({ storePath: indexStatusPath }); const first = await getMemorySearchManager({ cfg, agentId: "main" }); - expect(first.manager).not.toBeNull(); - if (!first.manager) { - throw new Error("manager missing"); - } - await first.manager.sync?.({ reason: "test" }); - await first.manager.close?.(); + const firstManager = requireManager(first); + await firstManager.sync?.({ reason: "test" }); + await firstManager.close?.(); const statusOnly = await getMemorySearchManager({ cfg, agentId: "main", purpose: "status", }); - expect(statusOnly.manager).not.toBeNull(); - if (!statusOnly.manager) { - throw new Error("status manager missing"); - } - - const status = statusOnly.manager.status(); + const statusManager = requireManager(statusOnly, "status manager missing"); + const status = statusManager.status(); expect(status.dirty).toBe(false); - await statusOnly.manager.close?.(); + await statusManager.close?.(); }); it("reindexes sessions when source config adds sessions to an existing index", async () => { @@ -244,31 +257,25 @@ describe("memory index", () => { try { const first = await getMemorySearchManager({ cfg: firstCfg, agentId: "main" }); - expect(first.manager).not.toBeNull(); - if (!first.manager) { - throw new Error("manager missing"); - } - await first.manager.sync?.({ reason: "test" }); - const firstStatus = first.manager.status(); + const firstManager = requireManager(first); + await firstManager.sync?.({ reason: "test" }); + const firstStatus = firstManager.status(); expect( firstStatus.sourceCounts?.find((entry) => entry.source === "sessions")?.files ?? 0, ).toBe(0); - await first.manager.close?.(); + await firstManager.close?.(); const second = await getMemorySearchManager({ cfg: secondCfg, agentId: "main" }); - expect(second.manager).not.toBeNull(); - if (!second.manager) { - throw new Error("manager missing"); - } - await second.manager.sync?.({ reason: "test" }); - const secondStatus = second.manager.status(); + const secondManager = requireManager(second); + await secondManager.sync?.({ reason: "test" }); + const secondStatus = secondManager.status(); expect(secondStatus.sourceCounts?.find((entry) => entry.source === "sessions")?.files).toBe( 1, ); expect( secondStatus.sourceCounts?.find((entry) => entry.source === "sessions")?.chunks ?? 0, ).toBeGreaterThan(0); - await second.manager.close?.(); + await secondManager.close?.(); } finally { if (previousStateDir === undefined) { delete process.env.OPENCLAW_STATE_DIR; @@ -302,13 +309,10 @@ describe("memory index", () => { }, agentId: "main", }); - expect(first.manager).not.toBeNull(); - if (!first.manager) { - throw new Error("manager missing"); - } - await first.manager.sync?.({ reason: "test" }); + const firstManager = requireManager(first); + await firstManager.sync?.({ reason: "test" }); const callsAfterFirstSync = embedBatchCalls; - await first.manager.close?.(); + await firstManager.close?.(); const second = await getMemorySearchManager({ cfg: { @@ -326,15 +330,12 @@ describe("memory index", () => { }, agentId: "main", }); - expect(second.manager).not.toBeNull(); - if (!second.manager) { - throw new Error("manager missing"); - } - await second.manager.sync?.({ reason: "test" }); + const secondManager = requireManager(second); + await secondManager.sync?.({ reason: "test" }); expect(embedBatchCalls).toBeGreaterThan(callsAfterFirstSync); - const status = second.manager.status(); + const status = secondManager.status(); expect(status.files).toBeGreaterThan(0); - await second.manager.close?.(); + await secondManager.close?.(); }); it("reuses cached embeddings on forced reindex", async () => { @@ -351,40 +352,22 @@ describe("memory index", () => { }); it("finds keyword matches via hybrid search when query embedding is zero", async () => { - const cfg = createCfg({ - storePath: indexMainPath, - hybrid: { enabled: true, vectorWeight: 0, textWeight: 1 }, - }); - const manager = await getPersistentManager(cfg); - - const status = manager.status(); - if (!status.fts?.available) { - return; - } - - await manager.sync({ reason: "test" }); - const results = await manager.search("zebra"); - expect(results.length).toBeGreaterThan(0); - expect(results[0]?.path).toContain("memory/2026-01-12.md"); + await expectHybridKeywordSearchFindsMemory( + createCfg({ + storePath: indexMainPath, + hybrid: { enabled: true, vectorWeight: 0, textWeight: 1 }, + }), + ); }); it("preserves keyword-only hybrid hits when minScore exceeds text weight", async () => { - const cfg = createCfg({ - storePath: indexMainPath, - minScore: 0.35, - hybrid: { enabled: true, vectorWeight: 0.7, textWeight: 0.3 }, - }); - const manager = await getPersistentManager(cfg); - - const status = manager.status(); - if (!status.fts?.available) { - return; - } - - await manager.sync({ reason: "test" }); - const results = await manager.search("zebra"); - expect(results.length).toBeGreaterThan(0); - expect(results[0]?.path).toContain("memory/2026-01-12.md"); + await expectHybridKeywordSearchFindsMemory( + createCfg({ + storePath: indexMainPath, + minScore: 0.35, + hybrid: { enabled: true, vectorWeight: 0.7, textWeight: 0.3 }, + }), + ); }); it("reports vector availability after probe", async () => { diff --git a/src/memory/manager.readonly-recovery.test.ts b/src/memory/manager.readonly-recovery.test.ts index 052ec9f24e0..35f37cf8371 100644 --- a/src/memory/manager.readonly-recovery.test.ts +++ b/src/memory/manager.readonly-recovery.test.ts @@ -13,6 +13,51 @@ describe("memory manager readonly recovery", () => { let indexPath = ""; let manager: MemoryIndexManager | null = null; + function createMemoryConfig(): OpenClawConfig { + return { + agents: { + defaults: { + workspace: workspaceDir, + memorySearch: { + provider: "openai", + model: "mock-embed", + store: { path: indexPath }, + sync: { watch: false, onSessionStart: false, onSearch: false }, + }, + }, + list: [{ id: "main", default: true }], + }, + } as OpenClawConfig; + } + + async function createManager() { + manager = await getRequiredMemoryIndexManager({ cfg: createMemoryConfig(), agentId: "main" }); + return manager; + } + + function createSyncSpies(instance: MemoryIndexManager) { + const runSyncSpy = vi.spyOn( + instance as unknown as { + runSync: (params?: { reason?: string; force?: boolean }) => Promise; + }, + "runSync", + ); + const openDatabaseSpy = vi.spyOn( + instance as unknown as { openDatabase: () => DatabaseSync }, + "openDatabase", + ); + return { runSyncSpy, openDatabaseSpy }; + } + + function expectReadonlyRecoveryStatus(lastError: string) { + expect(manager?.status().custom?.readonlyRecovery).toEqual({ + attempts: 1, + successes: 1, + failures: 0, + lastError, + }); + } + beforeEach(async () => { resetEmbeddingMocks(); workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-mem-readonly-")); @@ -30,124 +75,39 @@ describe("memory manager readonly recovery", () => { }); it("reopens sqlite and retries once when sync hits SQLITE_READONLY", async () => { - const cfg = { - agents: { - defaults: { - workspace: workspaceDir, - memorySearch: { - provider: "openai", - model: "mock-embed", - store: { path: indexPath }, - sync: { watch: false, onSessionStart: false, onSearch: false }, - }, - }, - list: [{ id: "main", default: true }], - }, - } as OpenClawConfig; - - manager = await getRequiredMemoryIndexManager({ cfg, agentId: "main" }); - - const runSyncSpy = vi.spyOn( - manager as unknown as { - runSync: (params?: { reason?: string; force?: boolean }) => Promise; - }, - "runSync", - ); + const currentManager = await createManager(); + const { runSyncSpy, openDatabaseSpy } = createSyncSpies(currentManager); runSyncSpy .mockRejectedValueOnce(new Error("attempt to write a readonly database")) .mockResolvedValueOnce(undefined); - const openDatabaseSpy = vi.spyOn( - manager as unknown as { openDatabase: () => DatabaseSync }, - "openDatabase", - ); - await manager.sync({ reason: "test" }); + await currentManager.sync({ reason: "test" }); expect(runSyncSpy).toHaveBeenCalledTimes(2); expect(openDatabaseSpy).toHaveBeenCalledTimes(1); - expect(manager.status().custom?.readonlyRecovery).toEqual({ - attempts: 1, - successes: 1, - failures: 0, - lastError: "attempt to write a readonly database", - }); + expectReadonlyRecoveryStatus("attempt to write a readonly database"); }); it("reopens sqlite and retries when readonly appears in error code", async () => { - const cfg = { - agents: { - defaults: { - workspace: workspaceDir, - memorySearch: { - provider: "openai", - model: "mock-embed", - store: { path: indexPath }, - sync: { watch: false, onSessionStart: false, onSearch: false }, - }, - }, - list: [{ id: "main", default: true }], - }, - } as OpenClawConfig; - - manager = await getRequiredMemoryIndexManager({ cfg, agentId: "main" }); - - const runSyncSpy = vi.spyOn( - manager as unknown as { - runSync: (params?: { reason?: string; force?: boolean }) => Promise; - }, - "runSync", - ); + const currentManager = await createManager(); + const { runSyncSpy, openDatabaseSpy } = createSyncSpies(currentManager); runSyncSpy .mockRejectedValueOnce({ message: "write failed", code: "SQLITE_READONLY" }) .mockResolvedValueOnce(undefined); - const openDatabaseSpy = vi.spyOn( - manager as unknown as { openDatabase: () => DatabaseSync }, - "openDatabase", - ); - await manager.sync({ reason: "test" }); + await currentManager.sync({ reason: "test" }); expect(runSyncSpy).toHaveBeenCalledTimes(2); expect(openDatabaseSpy).toHaveBeenCalledTimes(1); - expect(manager.status().custom?.readonlyRecovery).toEqual({ - attempts: 1, - successes: 1, - failures: 0, - lastError: "write failed", - }); + expectReadonlyRecoveryStatus("write failed"); }); it("does not retry non-readonly sync errors", async () => { - const cfg = { - agents: { - defaults: { - workspace: workspaceDir, - memorySearch: { - provider: "openai", - model: "mock-embed", - store: { path: indexPath }, - sync: { watch: false, onSessionStart: false, onSearch: false }, - }, - }, - list: [{ id: "main", default: true }], - }, - } as OpenClawConfig; - - manager = await getRequiredMemoryIndexManager({ cfg, agentId: "main" }); - - const runSyncSpy = vi.spyOn( - manager as unknown as { - runSync: (params?: { reason?: string; force?: boolean }) => Promise; - }, - "runSync", - ); + const currentManager = await createManager(); + const { runSyncSpy, openDatabaseSpy } = createSyncSpies(currentManager); runSyncSpy.mockRejectedValueOnce(new Error("embedding timeout")); - const openDatabaseSpy = vi.spyOn( - manager as unknown as { openDatabase: () => DatabaseSync }, - "openDatabase", - ); - await expect(manager.sync({ reason: "test" })).rejects.toThrow("embedding timeout"); + await expect(currentManager.sync({ reason: "test" })).rejects.toThrow("embedding timeout"); expect(runSyncSpy).toHaveBeenCalledTimes(1); expect(openDatabaseSpy).toHaveBeenCalledTimes(0); }); diff --git a/src/plugins/discovery.test.ts b/src/plugins/discovery.test.ts index 68cd0c83915..806411c3a94 100644 --- a/src/plugins/discovery.test.ts +++ b/src/plugins/discovery.test.ts @@ -26,6 +26,27 @@ async function withStateDir(stateDir: string, fn: () => Promise) { ); } +function writePluginPackageManifest(params: { + packageDir: string; + packageName: string; + extensions: string[]; +}) { + fs.writeFileSync( + path.join(params.packageDir, "package.json"), + JSON.stringify({ + name: params.packageName, + openclaw: { extensions: params.extensions }, + }), + "utf-8", + ); +} + +function expectEscapesPackageDiagnostic(diagnostics: Array<{ message: string }>) { + expect(diagnostics.some((entry) => entry.message.includes("escapes package directory"))).toBe( + true, + ); +} + afterEach(() => { for (const dir of tempDirs.splice(0)) { try { @@ -95,14 +116,11 @@ describe("discoverOpenClawPlugins", () => { const globalExt = path.join(stateDir, "extensions", "pack"); fs.mkdirSync(path.join(globalExt, "src"), { recursive: true }); - fs.writeFileSync( - path.join(globalExt, "package.json"), - JSON.stringify({ - name: "pack", - openclaw: { extensions: ["./src/one.ts", "./src/two.ts"] }, - }), - "utf-8", - ); + writePluginPackageManifest({ + packageDir: globalExt, + packageName: "pack", + extensions: ["./src/one.ts", "./src/two.ts"], + }); fs.writeFileSync( path.join(globalExt, "src", "one.ts"), "export default function () {}", @@ -128,14 +146,11 @@ describe("discoverOpenClawPlugins", () => { const globalExt = path.join(stateDir, "extensions", "voice-call-pack"); fs.mkdirSync(path.join(globalExt, "src"), { recursive: true }); - fs.writeFileSync( - path.join(globalExt, "package.json"), - JSON.stringify({ - name: "@openclaw/voice-call", - openclaw: { extensions: ["./src/index.ts"] }, - }), - "utf-8", - ); + writePluginPackageManifest({ + packageDir: globalExt, + packageName: "@openclaw/voice-call", + extensions: ["./src/index.ts"], + }); fs.writeFileSync( path.join(globalExt, "src", "index.ts"), "export default function () {}", @@ -155,14 +170,11 @@ describe("discoverOpenClawPlugins", () => { const packDir = path.join(stateDir, "packs", "demo-plugin-dir"); fs.mkdirSync(packDir, { recursive: true }); - fs.writeFileSync( - path.join(packDir, "package.json"), - JSON.stringify({ - name: "@openclaw/demo-plugin-dir", - openclaw: { extensions: ["./index.js"] }, - }), - "utf-8", - ); + writePluginPackageManifest({ + packageDir: packDir, + packageName: "@openclaw/demo-plugin-dir", + extensions: ["./index.js"], + }); fs.writeFileSync(path.join(packDir, "index.js"), "module.exports = {}", "utf-8"); const { candidates } = await withStateDir(stateDir, async () => { @@ -178,14 +190,11 @@ describe("discoverOpenClawPlugins", () => { const outside = path.join(stateDir, "outside.js"); fs.mkdirSync(globalExt, { recursive: true }); - fs.writeFileSync( - path.join(globalExt, "package.json"), - JSON.stringify({ - name: "@openclaw/escape-pack", - openclaw: { extensions: ["../../outside.js"] }, - }), - "utf-8", - ); + writePluginPackageManifest({ + packageDir: globalExt, + packageName: "@openclaw/escape-pack", + extensions: ["../../outside.js"], + }); fs.writeFileSync(outside, "export default function () {}", "utf-8"); const result = await withStateDir(stateDir, async () => { @@ -193,9 +202,7 @@ describe("discoverOpenClawPlugins", () => { }); expect(result.candidates).toHaveLength(0); - expect( - result.diagnostics.some((diag) => diag.message.includes("escapes package directory")), - ).toBe(true); + expectEscapesPackageDiagnostic(result.diagnostics); }); it("rejects package extension entries that escape via symlink", async () => { @@ -212,23 +219,18 @@ describe("discoverOpenClawPlugins", () => { return; } - fs.writeFileSync( - path.join(globalExt, "package.json"), - JSON.stringify({ - name: "@openclaw/pack", - openclaw: { extensions: ["./linked/escape.ts"] }, - }), - "utf-8", - ); + writePluginPackageManifest({ + packageDir: globalExt, + packageName: "@openclaw/pack", + extensions: ["./linked/escape.ts"], + }); const { candidates, diagnostics } = await withStateDir(stateDir, async () => { return discoverOpenClawPlugins({}); }); expect(candidates.some((candidate) => candidate.idHint === "pack")).toBe(false); - expect(diagnostics.some((entry) => entry.message.includes("escapes package directory"))).toBe( - true, - ); + expectEscapesPackageDiagnostic(diagnostics); }); it("rejects package extension entries that are hardlinked aliases", async () => { @@ -252,23 +254,18 @@ describe("discoverOpenClawPlugins", () => { throw err; } - fs.writeFileSync( - path.join(globalExt, "package.json"), - JSON.stringify({ - name: "@openclaw/pack", - openclaw: { extensions: ["./escape.ts"] }, - }), - "utf-8", - ); + writePluginPackageManifest({ + packageDir: globalExt, + packageName: "@openclaw/pack", + extensions: ["./escape.ts"], + }); const { candidates, diagnostics } = await withStateDir(stateDir, async () => { return discoverOpenClawPlugins({}); }); expect(candidates.some((candidate) => candidate.idHint === "pack")).toBe(false); - expect(diagnostics.some((entry) => entry.message.includes("escapes package directory"))).toBe( - true, - ); + expectEscapesPackageDiagnostic(diagnostics); }); it("ignores package manifests that are hardlinked aliases", async () => { diff --git a/src/plugins/install.test.ts b/src/plugins/install.test.ts index 9f67e69430b..6e3b40bd212 100644 --- a/src/plugins/install.test.ts +++ b/src/plugins/install.test.ts @@ -158,6 +158,19 @@ function expectPluginFiles(result: { targetDir: string }, stateDir: string, plug expect(fs.existsSync(path.join(result.targetDir, "dist", "index.js"))).toBe(true); } +function expectSuccessfulArchiveInstall(params: { + result: Awaited>; + stateDir: string; + pluginId: string; +}) { + expect(params.result.ok).toBe(true); + if (!params.result.ok) { + return; + } + expect(params.result.pluginId).toBe(params.pluginId); + expectPluginFiles(params.result, params.stateDir, params.pluginId); +} + function setupPluginInstallDirs() { const tmpDir = makeTempDir(); const pluginDir = path.join(tmpDir, "plugin-src"); @@ -200,6 +213,30 @@ async function installFromDirWithWarnings(params: { pluginDir: string; extension return { result, warnings }; } +function setupManifestInstallFixture(params: { manifestId: string }) { + const { pluginDir, extensionsDir } = setupPluginInstallDirs(); + fs.mkdirSync(path.join(pluginDir, "dist"), { recursive: true }); + fs.writeFileSync( + path.join(pluginDir, "package.json"), + JSON.stringify({ + name: "@openclaw/cognee-openclaw", + version: "0.0.1", + openclaw: { extensions: ["./dist/index.js"] }, + }), + "utf-8", + ); + fs.writeFileSync(path.join(pluginDir, "dist", "index.js"), "export {};", "utf-8"); + fs.writeFileSync( + path.join(pluginDir, "openclaw.plugin.json"), + JSON.stringify({ + id: params.manifestId, + configSchema: { type: "object", properties: {} }, + }), + "utf-8", + ); + return { pluginDir, extensionsDir }; +} + async function expectArchiveInstallReservedSegmentRejection(params: { packageName: string; outName: string; @@ -281,12 +318,7 @@ describe("installPluginFromArchive", () => { archivePath, extensionsDir, }); - expect(result.ok).toBe(true); - if (!result.ok) { - return; - } - expect(result.pluginId).toBe("voice-call"); - expectPluginFiles(result, stateDir, "voice-call"); + expectSuccessfulArchiveInstall({ result, stateDir, pluginId: "voice-call" }); }); it("rejects installing when plugin already exists", async () => { @@ -324,13 +356,7 @@ describe("installPluginFromArchive", () => { archivePath, extensionsDir, }); - - expect(result.ok).toBe(true); - if (!result.ok) { - return; - } - expect(result.pluginId).toBe("zipper"); - expectPluginFiles(result, stateDir, "zipper"); + expectSuccessfulArchiveInstall({ result, stateDir, pluginId: "zipper" }); }); it("allows updates when mode is update", async () => { @@ -515,26 +541,9 @@ describe("installPluginFromDir", () => { }); it("uses openclaw.plugin.json id as install key when it differs from package name", async () => { - const { pluginDir, extensionsDir } = setupPluginInstallDirs(); - fs.mkdirSync(path.join(pluginDir, "dist"), { recursive: true }); - fs.writeFileSync( - path.join(pluginDir, "package.json"), - JSON.stringify({ - name: "@openclaw/cognee-openclaw", - version: "0.0.1", - openclaw: { extensions: ["./dist/index.js"] }, - }), - "utf-8", - ); - fs.writeFileSync(path.join(pluginDir, "dist", "index.js"), "export {};", "utf-8"); - fs.writeFileSync( - path.join(pluginDir, "openclaw.plugin.json"), - JSON.stringify({ - id: "memory-cognee", - configSchema: { type: "object", properties: {} }, - }), - "utf-8", - ); + const { pluginDir, extensionsDir } = setupManifestInstallFixture({ + manifestId: "memory-cognee", + }); const infoMessages: string[] = []; const res = await installPluginFromDir({ @@ -559,26 +568,9 @@ describe("installPluginFromDir", () => { }); it("normalizes scoped manifest ids to unscoped install keys", async () => { - const { pluginDir, extensionsDir } = setupPluginInstallDirs(); - fs.mkdirSync(path.join(pluginDir, "dist"), { recursive: true }); - fs.writeFileSync( - path.join(pluginDir, "package.json"), - JSON.stringify({ - name: "@openclaw/cognee-openclaw", - version: "0.0.1", - openclaw: { extensions: ["./dist/index.js"] }, - }), - "utf-8", - ); - fs.writeFileSync(path.join(pluginDir, "dist", "index.js"), "export {};", "utf-8"); - fs.writeFileSync( - path.join(pluginDir, "openclaw.plugin.json"), - JSON.stringify({ - id: "@team/memory-cognee", - configSchema: { type: "object", properties: {} }, - }), - "utf-8", - ); + const { pluginDir, extensionsDir } = setupManifestInstallFixture({ + manifestId: "@team/memory-cognee", + }); const res = await installPluginFromDir({ dirPath: pluginDir, diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index ffa5be4be7d..1802f33abee 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -132,6 +132,70 @@ function expectTelegramLoaded(registry: ReturnType) expect(registry.channels.some((entry) => entry.plugin.id === "telegram")).toBe(true); } +function useNoBundledPlugins() { + process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; +} + +function loadRegistryFromSinglePlugin(params: { + plugin: TempPlugin; + pluginConfig?: Record; + includeWorkspaceDir?: boolean; + options?: Omit[0], "cache" | "workspaceDir" | "config">; +}) { + const pluginConfig = params.pluginConfig ?? {}; + return loadOpenClawPlugins({ + cache: false, + ...(params.includeWorkspaceDir === false ? {} : { workspaceDir: params.plugin.dir }), + ...params.options, + config: { + plugins: { + load: { paths: [params.plugin.file] }, + ...pluginConfig, + }, + }, + }); +} + +function createWarningLogger(warnings: string[]) { + return { + info: () => {}, + warn: (msg: string) => warnings.push(msg), + error: () => {}, + }; +} + +function createEscapingEntryFixture(params: { id: string; sourceBody: string }) { + const pluginDir = makeTempDir(); + const outsideDir = makeTempDir(); + const outsideEntry = path.join(outsideDir, "outside.js"); + const linkedEntry = path.join(pluginDir, "entry.js"); + fs.writeFileSync(outsideEntry, params.sourceBody, "utf-8"); + fs.writeFileSync( + path.join(pluginDir, "openclaw.plugin.json"), + JSON.stringify( + { + id: params.id, + configSchema: EMPTY_PLUGIN_SCHEMA, + }, + null, + 2, + ), + "utf-8", + ); + return { pluginDir, outsideEntry, linkedEntry }; +} + +function createPluginSdkAliasFixture() { + const root = makeTempDir(); + const srcFile = path.join(root, "src", "plugin-sdk", "index.ts"); + const distFile = path.join(root, "dist", "plugin-sdk", "index.js"); + fs.mkdirSync(path.dirname(srcFile), { recursive: true }); + fs.mkdirSync(path.dirname(distFile), { recursive: true }); + fs.writeFileSync(srcFile, "export {};\n", "utf-8"); + fs.writeFileSync(distFile, "export {};\n", "utf-8"); + return { root, srcFile, distFile }; +} + afterEach(() => { if (prevBundledDir === undefined) { delete process.env.OPENCLAW_BUNDLED_PLUGINS_DIR; @@ -327,7 +391,7 @@ describe("loadOpenClawPlugins", () => { }); it("loads plugins when source and root differ only by realpath alias", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "alias-safe", body: `export default { id: "alias-safe", register() {} };`, @@ -337,14 +401,10 @@ describe("loadOpenClawPlugins", () => { return; } - const registry = loadOpenClawPlugins({ - cache: false, - workspaceDir: plugin.dir, - config: { - plugins: { - load: { paths: [plugin.file] }, - allow: ["alias-safe"], - }, + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["alias-safe"], }, }); @@ -353,21 +413,17 @@ describe("loadOpenClawPlugins", () => { }); it("denylist disables plugins even if allowed", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "blocked", body: `export default { id: "blocked", register() {} };`, }); - const registry = loadOpenClawPlugins({ - cache: false, - workspaceDir: plugin.dir, - config: { - plugins: { - load: { paths: [plugin.file] }, - allow: ["blocked"], - deny: ["blocked"], - }, + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["blocked"], + deny: ["blocked"], }, }); @@ -376,22 +432,18 @@ describe("loadOpenClawPlugins", () => { }); it("fails fast on invalid plugin config", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "configurable", body: `export default { id: "configurable", register() {} };`, }); - const registry = loadOpenClawPlugins({ - cache: false, - workspaceDir: plugin.dir, - config: { - plugins: { - load: { paths: [plugin.file] }, - entries: { - configurable: { - config: "nope" as unknown as Record, - }, + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + entries: { + configurable: { + config: "nope" as unknown as Record, }, }, }, @@ -403,7 +455,7 @@ describe("loadOpenClawPlugins", () => { }); it("registers channel plugins", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "channel-demo", body: `export default { id: "channel-demo", register(api) { @@ -428,14 +480,10 @@ describe("loadOpenClawPlugins", () => { } };`, }); - const registry = loadOpenClawPlugins({ - cache: false, - workspaceDir: plugin.dir, - config: { - plugins: { - load: { paths: [plugin.file] }, - allow: ["channel-demo"], - }, + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["channel-demo"], }, }); @@ -444,7 +492,7 @@ describe("loadOpenClawPlugins", () => { }); it("registers http handlers", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "http-demo", body: `export default { id: "http-demo", register(api) { @@ -452,14 +500,10 @@ describe("loadOpenClawPlugins", () => { } };`, }); - const registry = loadOpenClawPlugins({ - cache: false, - workspaceDir: plugin.dir, - config: { - plugins: { - load: { paths: [plugin.file] }, - allow: ["http-demo"], - }, + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["http-demo"], }, }); @@ -470,7 +514,7 @@ describe("loadOpenClawPlugins", () => { }); it("registers http routes", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "http-route-demo", body: `export default { id: "http-route-demo", register(api) { @@ -478,14 +522,10 @@ describe("loadOpenClawPlugins", () => { } };`, }); - const registry = loadOpenClawPlugins({ - cache: false, - workspaceDir: plugin.dir, - config: { - plugins: { - load: { paths: [plugin.file] }, - allow: ["http-route-demo"], - }, + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["http-route-demo"], }, }); @@ -644,7 +684,7 @@ describe("loadOpenClawPlugins", () => { }); it("warns when plugins.allow is empty and non-bundled plugins are discoverable", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const plugin = writePlugin({ id: "warn-open-allow", body: `export default { id: "warn-open-allow", register() {} };`, @@ -652,11 +692,7 @@ describe("loadOpenClawPlugins", () => { const warnings: string[] = []; loadOpenClawPlugins({ cache: false, - logger: { - info: () => {}, - warn: (msg) => warnings.push(msg), - error: () => {}, - }, + logger: createWarningLogger(warnings), config: { plugins: { load: { paths: [plugin.file] }, @@ -669,7 +705,7 @@ describe("loadOpenClawPlugins", () => { }); it("warns when loaded non-bundled plugin has no install/load-path provenance", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; + useNoBundledPlugins(); const stateDir = makeTempDir(); withEnv({ OPENCLAW_STATE_DIR: stateDir, CLAWDBOT_STATE_DIR: undefined }, () => { const globalDir = path.join(stateDir, "extensions", "rogue"); @@ -684,11 +720,7 @@ describe("loadOpenClawPlugins", () => { const warnings: string[] = []; const registry = loadOpenClawPlugins({ cache: false, - logger: { - info: () => {}, - warn: (msg) => warnings.push(msg), - error: () => {}, - }, + logger: createWarningLogger(warnings), config: { plugins: { allow: ["rogue"], @@ -708,28 +740,12 @@ describe("loadOpenClawPlugins", () => { }); it("rejects plugin entry files that escape plugin root via symlink", () => { - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; - const pluginDir = makeTempDir(); - const outsideDir = makeTempDir(); - const outsideEntry = path.join(outsideDir, "outside.js"); - const linkedEntry = path.join(pluginDir, "entry.js"); - fs.writeFileSync( - outsideEntry, - 'export default { id: "symlinked", register() { throw new Error("should not run"); } };', - "utf-8", - ); - fs.writeFileSync( - path.join(pluginDir, "openclaw.plugin.json"), - JSON.stringify( - { - id: "symlinked", - configSchema: EMPTY_PLUGIN_SCHEMA, - }, - null, - 2, - ), - "utf-8", - ); + useNoBundledPlugins(); + const { outsideEntry, linkedEntry } = createEscapingEntryFixture({ + id: "symlinked", + sourceBody: + 'export default { id: "symlinked", register() { throw new Error("should not run"); } };', + }); try { fs.symlinkSync(outsideEntry, linkedEntry); } catch { @@ -755,28 +771,12 @@ describe("loadOpenClawPlugins", () => { if (process.platform === "win32") { return; } - process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins"; - const pluginDir = makeTempDir(); - const outsideDir = makeTempDir(); - const outsideEntry = path.join(outsideDir, "outside.js"); - const linkedEntry = path.join(pluginDir, "entry.js"); - fs.writeFileSync( - outsideEntry, - 'export default { id: "hardlinked", register() { throw new Error("should not run"); } };', - "utf-8", - ); - fs.writeFileSync( - path.join(pluginDir, "openclaw.plugin.json"), - JSON.stringify( - { - id: "hardlinked", - configSchema: EMPTY_PLUGIN_SCHEMA, - }, - null, - 2, - ), - "utf-8", - ); + useNoBundledPlugins(); + const { outsideEntry, linkedEntry } = createEscapingEntryFixture({ + id: "hardlinked", + sourceBody: + 'export default { id: "hardlinked", register() { throw new Error("should not run"); } };', + }); try { fs.linkSync(outsideEntry, linkedEntry); } catch (err) { @@ -802,13 +802,7 @@ describe("loadOpenClawPlugins", () => { }); it("prefers dist plugin-sdk alias when loader runs from dist", () => { - const root = makeTempDir(); - const srcFile = path.join(root, "src", "plugin-sdk", "index.ts"); - const distFile = path.join(root, "dist", "plugin-sdk", "index.js"); - fs.mkdirSync(path.dirname(srcFile), { recursive: true }); - fs.mkdirSync(path.dirname(distFile), { recursive: true }); - fs.writeFileSync(srcFile, "export {};\n", "utf-8"); - fs.writeFileSync(distFile, "export {};\n", "utf-8"); + const { root, distFile } = createPluginSdkAliasFixture(); const resolved = __testing.resolvePluginSdkAliasFile({ srcFile: "index.ts", @@ -819,13 +813,7 @@ describe("loadOpenClawPlugins", () => { }); it("prefers src plugin-sdk alias when loader runs from src in non-production", () => { - const root = makeTempDir(); - const srcFile = path.join(root, "src", "plugin-sdk", "index.ts"); - const distFile = path.join(root, "dist", "plugin-sdk", "index.js"); - fs.mkdirSync(path.dirname(srcFile), { recursive: true }); - fs.mkdirSync(path.dirname(distFile), { recursive: true }); - fs.writeFileSync(srcFile, "export {};\n", "utf-8"); - fs.writeFileSync(distFile, "export {};\n", "utf-8"); + const { root, srcFile } = createPluginSdkAliasFixture(); const resolved = withEnv({ NODE_ENV: undefined }, () => __testing.resolvePluginSdkAliasFile({ diff --git a/src/secrets/apply.test.ts b/src/secrets/apply.test.ts index 3395d6411b3..f61f77e79f6 100644 --- a/src/secrets/apply.test.ts +++ b/src/secrets/apply.test.ts @@ -5,6 +5,22 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { runSecretsApply } from "./apply.js"; import type { SecretsApplyPlan } from "./plan.js"; +const OPENAI_API_KEY_ENV_REF = { + source: "env", + provider: "default", + id: "OPENAI_API_KEY", +} as const; + +type ApplyFixture = { + rootDir: string; + stateDir: string; + configPath: string; + authStorePath: string; + authJsonPath: string; + envPath: string; + env: NodeJS.ProcessEnv; +}; + function stripVolatileConfigMeta(input: string): Record { const parsed = JSON.parse(input) as Record; const meta = @@ -20,404 +36,322 @@ function stripVolatileConfigMeta(input: string): Record { return parsed; } +async function writeJsonFile(filePath: string, value: unknown): Promise { + await fs.writeFile(filePath, `${JSON.stringify(value, null, 2)}\n`, "utf8"); +} + +function createOpenAiProviderConfig(apiKey: unknown = "sk-openai-plaintext") { + return { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey, + models: [{ id: "gpt-5", name: "gpt-5" }], + }; +} + +function buildFixturePaths(rootDir: string) { + const stateDir = path.join(rootDir, ".openclaw"); + return { + rootDir, + stateDir, + configPath: path.join(stateDir, "openclaw.json"), + authStorePath: path.join(stateDir, "agents", "main", "agent", "auth-profiles.json"), + authJsonPath: path.join(stateDir, "agents", "main", "agent", "auth.json"), + envPath: path.join(stateDir, ".env"), + }; +} + +async function createApplyFixture(): Promise { + const paths = buildFixturePaths( + await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-secrets-apply-")), + ); + await fs.mkdir(path.dirname(paths.configPath), { recursive: true }); + await fs.mkdir(path.dirname(paths.authStorePath), { recursive: true }); + return { + ...paths, + env: { + OPENCLAW_STATE_DIR: paths.stateDir, + OPENCLAW_CONFIG_PATH: paths.configPath, + OPENAI_API_KEY: "sk-live-env", + }, + }; +} + +async function seedDefaultApplyFixture(fixture: ApplyFixture): Promise { + await writeJsonFile(fixture.configPath, { + models: { + providers: { + openai: createOpenAiProviderConfig(), + }, + }, + }); + await writeJsonFile(fixture.authStorePath, { + version: 1, + profiles: { + "openai:default": { + type: "api_key", + provider: "openai", + key: "sk-openai-plaintext", + }, + }, + }); + await writeJsonFile(fixture.authJsonPath, { + openai: { + type: "api_key", + key: "sk-openai-plaintext", + }, + }); + await fs.writeFile( + fixture.envPath, + "OPENAI_API_KEY=sk-openai-plaintext\nUNRELATED=value\n", + "utf8", + ); +} + +async function applyPlanAndReadConfig( + fixture: ApplyFixture, + plan: SecretsApplyPlan, +): Promise { + const result = await runSecretsApply({ plan, env: fixture.env, write: true }); + expect(result.changed).toBe(true); + return JSON.parse(await fs.readFile(fixture.configPath, "utf8")) as T; +} + +async function expectInvalidTargetPath( + fixture: ApplyFixture, + target: SecretsApplyPlan["targets"][number], +): Promise { + const plan = createPlan({ targets: [target] }); + await expect(runSecretsApply({ plan, env: fixture.env, write: false })).rejects.toThrow( + "Invalid plan target path", + ); +} + +function createPlan(params: { + targets: SecretsApplyPlan["targets"]; + options?: SecretsApplyPlan["options"]; + providerUpserts?: SecretsApplyPlan["providerUpserts"]; + providerDeletes?: SecretsApplyPlan["providerDeletes"]; +}): SecretsApplyPlan { + return { + version: 1, + protocolVersion: 1, + generatedAt: new Date().toISOString(), + generatedBy: "manual", + targets: params.targets, + ...(params.options ? { options: params.options } : {}), + ...(params.providerUpserts ? { providerUpserts: params.providerUpserts } : {}), + ...(params.providerDeletes ? { providerDeletes: params.providerDeletes } : {}), + }; +} + +function createOpenAiProviderTarget(params?: { + path?: string; + pathSegments?: string[]; + providerId?: string; +}): SecretsApplyPlan["targets"][number] { + return { + type: "models.providers.apiKey", + path: params?.path ?? "models.providers.openai.apiKey", + ...(params?.pathSegments ? { pathSegments: params.pathSegments } : {}), + providerId: params?.providerId ?? "openai", + ref: OPENAI_API_KEY_ENV_REF, + }; +} + +function createOneWayScrubOptions(): NonNullable { + return { + scrubEnv: true, + scrubAuthProfilesForProviderTargets: true, + scrubLegacyAuthJson: true, + }; +} + describe("secrets apply", () => { - let rootDir = ""; - let stateDir = ""; - let configPath = ""; - let authStorePath = ""; - let authJsonPath = ""; - let envPath = ""; - let env: NodeJS.ProcessEnv; + let fixture: ApplyFixture; beforeEach(async () => { - rootDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-secrets-apply-")); - stateDir = path.join(rootDir, ".openclaw"); - configPath = path.join(stateDir, "openclaw.json"); - authStorePath = path.join(stateDir, "agents", "main", "agent", "auth-profiles.json"); - authJsonPath = path.join(stateDir, "agents", "main", "agent", "auth.json"); - envPath = path.join(stateDir, ".env"); - env = { - OPENCLAW_STATE_DIR: stateDir, - OPENCLAW_CONFIG_PATH: configPath, - OPENAI_API_KEY: "sk-live-env", - }; - - await fs.mkdir(path.dirname(configPath), { recursive: true }); - await fs.mkdir(path.dirname(authStorePath), { recursive: true }); - - await fs.writeFile( - configPath, - `${JSON.stringify( - { - models: { - providers: { - openai: { - baseUrl: "https://api.openai.com/v1", - api: "openai-completions", - apiKey: "sk-openai-plaintext", - models: [{ id: "gpt-5", name: "gpt-5" }], - }, - }, - }, - }, - null, - 2, - )}\n`, - "utf8", - ); - - await fs.writeFile( - authStorePath, - `${JSON.stringify( - { - version: 1, - profiles: { - "openai:default": { - type: "api_key", - provider: "openai", - key: "sk-openai-plaintext", - }, - }, - }, - null, - 2, - )}\n`, - "utf8", - ); - - await fs.writeFile( - authJsonPath, - `${JSON.stringify( - { - openai: { - type: "api_key", - key: "sk-openai-plaintext", - }, - }, - null, - 2, - )}\n`, - "utf8", - ); - await fs.writeFile(envPath, "OPENAI_API_KEY=sk-openai-plaintext\nUNRELATED=value\n", "utf8"); + fixture = await createApplyFixture(); + await seedDefaultApplyFixture(fixture); }); afterEach(async () => { - await fs.rm(rootDir, { recursive: true, force: true }); + await fs.rm(fixture.rootDir, { recursive: true, force: true }); }); it("preflights and applies one-way scrub without plaintext backups", async () => { - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", - targets: [ - { - type: "models.providers.apiKey", - path: "models.providers.openai.apiKey", - providerId: "openai", - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - }, - ], - options: { - scrubEnv: true, - scrubAuthProfilesForProviderTargets: true, - scrubLegacyAuthJson: true, - }, - }; + const plan = createPlan({ + targets: [createOpenAiProviderTarget()], + options: createOneWayScrubOptions(), + }); - const dryRun = await runSecretsApply({ plan, env, write: false }); + const dryRun = await runSecretsApply({ plan, env: fixture.env, write: false }); expect(dryRun.mode).toBe("dry-run"); expect(dryRun.changed).toBe(true); - const applied = await runSecretsApply({ plan, env, write: true }); + const applied = await runSecretsApply({ plan, env: fixture.env, write: true }); expect(applied.mode).toBe("write"); expect(applied.changed).toBe(true); - const nextConfig = JSON.parse(await fs.readFile(configPath, "utf8")) as { + const nextConfig = JSON.parse(await fs.readFile(fixture.configPath, "utf8")) as { models: { providers: { openai: { apiKey: unknown } } }; }; - expect(nextConfig.models.providers.openai.apiKey).toEqual({ - source: "env", - provider: "default", - id: "OPENAI_API_KEY", - }); + expect(nextConfig.models.providers.openai.apiKey).toEqual(OPENAI_API_KEY_ENV_REF); - const nextAuthStore = JSON.parse(await fs.readFile(authStorePath, "utf8")) as { + const nextAuthStore = JSON.parse(await fs.readFile(fixture.authStorePath, "utf8")) as { profiles: { "openai:default": { key?: string; keyRef?: unknown } }; }; expect(nextAuthStore.profiles["openai:default"].key).toBeUndefined(); expect(nextAuthStore.profiles["openai:default"].keyRef).toBeUndefined(); - const nextAuthJson = JSON.parse(await fs.readFile(authJsonPath, "utf8")) as Record< + const nextAuthJson = JSON.parse(await fs.readFile(fixture.authJsonPath, "utf8")) as Record< string, unknown >; expect(nextAuthJson.openai).toBeUndefined(); - const nextEnv = await fs.readFile(envPath, "utf8"); + const nextEnv = await fs.readFile(fixture.envPath, "utf8"); expect(nextEnv).not.toContain("sk-openai-plaintext"); expect(nextEnv).toContain("UNRELATED=value"); }); it("is idempotent on repeated write applies", async () => { - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", - targets: [ - { - type: "models.providers.apiKey", - path: "models.providers.openai.apiKey", - providerId: "openai", - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - }, - ], - options: { - scrubEnv: true, - scrubAuthProfilesForProviderTargets: true, - scrubLegacyAuthJson: true, - }, - }; + const plan = createPlan({ + targets: [createOpenAiProviderTarget()], + options: createOneWayScrubOptions(), + }); - const first = await runSecretsApply({ plan, env, write: true }); + const first = await runSecretsApply({ plan, env: fixture.env, write: true }); expect(first.changed).toBe(true); - const configAfterFirst = await fs.readFile(configPath, "utf8"); - const authStoreAfterFirst = await fs.readFile(authStorePath, "utf8"); - const authJsonAfterFirst = await fs.readFile(authJsonPath, "utf8"); - const envAfterFirst = await fs.readFile(envPath, "utf8"); + const configAfterFirst = await fs.readFile(fixture.configPath, "utf8"); + const authStoreAfterFirst = await fs.readFile(fixture.authStorePath, "utf8"); + const authJsonAfterFirst = await fs.readFile(fixture.authJsonPath, "utf8"); + const envAfterFirst = await fs.readFile(fixture.envPath, "utf8"); - // Second apply should be a true no-op and avoid file writes entirely. - await fs.chmod(configPath, 0o400); - await fs.chmod(authStorePath, 0o400); + await fs.chmod(fixture.configPath, 0o400); + await fs.chmod(fixture.authStorePath, 0o400); - const second = await runSecretsApply({ plan, env, write: true }); + const second = await runSecretsApply({ plan, env: fixture.env, write: true }); expect(second.mode).toBe("write"); - const configAfterSecond = await fs.readFile(configPath, "utf8"); + const configAfterSecond = await fs.readFile(fixture.configPath, "utf8"); expect(stripVolatileConfigMeta(configAfterSecond)).toEqual( stripVolatileConfigMeta(configAfterFirst), ); - await expect(fs.readFile(authStorePath, "utf8")).resolves.toBe(authStoreAfterFirst); - await expect(fs.readFile(authJsonPath, "utf8")).resolves.toBe(authJsonAfterFirst); - await expect(fs.readFile(envPath, "utf8")).resolves.toBe(envAfterFirst); + await expect(fs.readFile(fixture.authStorePath, "utf8")).resolves.toBe(authStoreAfterFirst); + await expect(fs.readFile(fixture.authJsonPath, "utf8")).resolves.toBe(authJsonAfterFirst); + await expect(fs.readFile(fixture.envPath, "utf8")).resolves.toBe(envAfterFirst); }); it("applies targets safely when map keys contain dots", async () => { - await fs.writeFile( - configPath, - `${JSON.stringify( - { - models: { - providers: { - "openai.dev": { - baseUrl: "https://api.openai.com/v1", - api: "openai-completions", - apiKey: "sk-openai-plaintext", - models: [{ id: "gpt-5", name: "gpt-5" }], - }, - }, - }, + await writeJsonFile(fixture.configPath, { + models: { + providers: { + "openai.dev": createOpenAiProviderConfig(), }, - null, - 2, - )}\n`, - "utf8", - ); + }, + }); - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", + const plan = createPlan({ targets: [ - { - type: "models.providers.apiKey", + createOpenAiProviderTarget({ path: "models.providers.openai.dev.apiKey", pathSegments: ["models", "providers", "openai.dev", "apiKey"], providerId: "openai.dev", - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - }, + }), ], options: { scrubEnv: false, scrubAuthProfilesForProviderTargets: false, scrubLegacyAuthJson: false, }, - }; + }); - const result = await runSecretsApply({ plan, env, write: true }); - expect(result.changed).toBe(true); - - const nextConfig = JSON.parse(await fs.readFile(configPath, "utf8")) as { + const nextConfig = await applyPlanAndReadConfig<{ models?: { providers?: Record; }; - }; - expect(nextConfig.models?.providers?.["openai.dev"]?.apiKey).toEqual({ - source: "env", - provider: "default", - id: "OPENAI_API_KEY", - }); + }>(fixture, plan); + expect(nextConfig.models?.providers?.["openai.dev"]?.apiKey).toEqual(OPENAI_API_KEY_ENV_REF); expect(nextConfig.models?.providers?.openai).toBeUndefined(); }); it("migrates skills entries apiKey targets alongside provider api keys", async () => { - await fs.writeFile( - configPath, - `${JSON.stringify( - { - models: { - providers: { - openai: { - baseUrl: "https://api.openai.com/v1", - api: "openai-completions", - apiKey: "sk-openai-plaintext", - models: [{ id: "gpt-5", name: "gpt-5" }], - }, - }, - }, - skills: { - entries: { - "qa-secret-test": { - enabled: true, - apiKey: "sk-skill-plaintext", - }, - }, + await writeJsonFile(fixture.configPath, { + models: { + providers: { + openai: createOpenAiProviderConfig(), + }, + }, + skills: { + entries: { + "qa-secret-test": { + enabled: true, + apiKey: "sk-skill-plaintext", }, }, - null, - 2, - )}\n`, - "utf8", - ); + }, + }); - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", + const plan = createPlan({ targets: [ - { - type: "models.providers.apiKey", - path: "models.providers.openai.apiKey", - pathSegments: ["models", "providers", "openai", "apiKey"], - providerId: "openai", - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - }, + createOpenAiProviderTarget({ pathSegments: ["models", "providers", "openai", "apiKey"] }), { type: "skills.entries.apiKey", path: "skills.entries.qa-secret-test.apiKey", pathSegments: ["skills", "entries", "qa-secret-test", "apiKey"], - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, + ref: OPENAI_API_KEY_ENV_REF, }, ], - options: { - scrubEnv: true, - scrubAuthProfilesForProviderTargets: true, - scrubLegacyAuthJson: true, - }, - }; + options: createOneWayScrubOptions(), + }); - const result = await runSecretsApply({ plan, env, write: true }); - expect(result.changed).toBe(true); - - const nextConfig = JSON.parse(await fs.readFile(configPath, "utf8")) as { + const nextConfig = await applyPlanAndReadConfig<{ models: { providers: { openai: { apiKey: unknown } } }; skills: { entries: { "qa-secret-test": { apiKey: unknown } } }; - }; - expect(nextConfig.models.providers.openai.apiKey).toEqual({ - source: "env", - provider: "default", - id: "OPENAI_API_KEY", - }); - expect(nextConfig.skills.entries["qa-secret-test"].apiKey).toEqual({ - source: "env", - provider: "default", - id: "OPENAI_API_KEY", - }); + }>(fixture, plan); + expect(nextConfig.models.providers.openai.apiKey).toEqual(OPENAI_API_KEY_ENV_REF); + expect(nextConfig.skills.entries["qa-secret-test"].apiKey).toEqual(OPENAI_API_KEY_ENV_REF); - const rawConfig = await fs.readFile(configPath, "utf8"); + const rawConfig = await fs.readFile(fixture.configPath, "utf8"); expect(rawConfig).not.toContain("sk-openai-plaintext"); expect(rawConfig).not.toContain("sk-skill-plaintext"); }); - it("rejects plan targets that do not match allowed secret-bearing paths", async () => { - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", - targets: [ - { - type: "models.providers.apiKey", - path: "models.providers.openai.baseUrl", - pathSegments: ["models", "providers", "openai", "baseUrl"], - providerId: "openai", - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - }, - ], - }; - - await expect(runSecretsApply({ plan, env, write: false })).rejects.toThrow( - "Invalid plan target path", - ); - }); - - it("rejects plan targets with forbidden prototype-like path segments", async () => { - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", - targets: [ - { - type: "skills.entries.apiKey", - path: "skills.entries.__proto__.apiKey", - pathSegments: ["skills", "entries", "__proto__", "apiKey"], - ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - }, - ], - }; - - await expect(runSecretsApply({ plan, env, write: false })).rejects.toThrow( - "Invalid plan target path", - ); + it.each([ + createOpenAiProviderTarget({ + path: "models.providers.openai.baseUrl", + pathSegments: ["models", "providers", "openai", "baseUrl"], + }), + { + type: "skills.entries.apiKey", + path: "skills.entries.__proto__.apiKey", + pathSegments: ["skills", "entries", "__proto__", "apiKey"], + ref: OPENAI_API_KEY_ENV_REF, + } satisfies SecretsApplyPlan["targets"][number], + ])("rejects invalid target path: %s", async (target) => { + await expectInvalidTargetPath(fixture, target); }); it("applies provider upserts and deletes from plan", async () => { - await fs.writeFile( - configPath, - `${JSON.stringify( - { - secrets: { - providers: { - envmain: { source: "env" }, - fileold: { source: "file", path: "/tmp/old-secrets.json", mode: "json" }, - }, - }, - models: { - providers: { - openai: { - baseUrl: "https://api.openai.com/v1", - api: "openai-completions", - models: [{ id: "gpt-5", name: "gpt-5" }], - }, - }, + await writeJsonFile(fixture.configPath, { + secrets: { + providers: { + envmain: { source: "env" }, + fileold: { source: "file", path: "/tmp/old-secrets.json", mode: "json" }, + }, + }, + models: { + providers: { + openai: { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + models: [{ id: "gpt-5", name: "gpt-5" }], }, }, - null, - 2, - )}\n`, - "utf8", - ); + }, + }); - const plan: SecretsApplyPlan = { - version: 1, - protocolVersion: 1, - generatedAt: new Date().toISOString(), - generatedBy: "manual", + const plan = createPlan({ providerUpserts: { filemain: { source: "file", @@ -427,16 +361,13 @@ describe("secrets apply", () => { }, providerDeletes: ["fileold"], targets: [], - }; + }); - const result = await runSecretsApply({ plan, env, write: true }); - expect(result.changed).toBe(true); - - const nextConfig = JSON.parse(await fs.readFile(configPath, "utf8")) as { + const nextConfig = await applyPlanAndReadConfig<{ secrets?: { providers?: Record; }; - }; + }>(fixture, plan); expect(nextConfig.secrets?.providers?.fileold).toBeUndefined(); expect(nextConfig.secrets?.providers?.filemain).toEqual({ source: "file", diff --git a/src/secrets/audit.test.ts b/src/secrets/audit.test.ts index 230bf62a042..97ba1aa677d 100644 --- a/src/secrets/audit.test.ts +++ b/src/secrets/audit.test.ts @@ -4,126 +4,142 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { runSecretsAudit } from "./audit.js"; -describe("secrets audit", () => { - let rootDir = ""; - let stateDir = ""; - let configPath = ""; - let authStorePath = ""; - let authJsonPath = ""; - let envPath = ""; - let env: NodeJS.ProcessEnv; +type AuditFixture = { + rootDir: string; + stateDir: string; + configPath: string; + authStorePath: string; + authJsonPath: string; + envPath: string; + env: NodeJS.ProcessEnv; +}; - beforeEach(async () => { - rootDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-secrets-audit-")); - stateDir = path.join(rootDir, ".openclaw"); - configPath = path.join(stateDir, "openclaw.json"); - authStorePath = path.join(stateDir, "agents", "main", "agent", "auth-profiles.json"); - authJsonPath = path.join(stateDir, "agents", "main", "agent", "auth.json"); - envPath = path.join(stateDir, ".env"); - env = { +async function writeJsonFile(filePath: string, value: unknown): Promise { + await fs.writeFile(filePath, `${JSON.stringify(value, null, 2)}\n`, "utf8"); +} + +function resolveRuntimePathEnv(): string { + if (typeof process.env.PATH === "string" && process.env.PATH.trim().length > 0) { + return process.env.PATH; + } + return "/usr/bin:/bin"; +} + +function hasFinding( + report: Awaited>, + predicate: (entry: { code: string; file: string }) => boolean, +): boolean { + return report.findings.some((entry) => predicate(entry as { code: string; file: string })); +} + +async function createAuditFixture(): Promise { + const rootDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-secrets-audit-")); + const stateDir = path.join(rootDir, ".openclaw"); + const configPath = path.join(stateDir, "openclaw.json"); + const authStorePath = path.join(stateDir, "agents", "main", "agent", "auth-profiles.json"); + const authJsonPath = path.join(stateDir, "agents", "main", "agent", "auth.json"); + const envPath = path.join(stateDir, ".env"); + + await fs.mkdir(path.dirname(configPath), { recursive: true }); + await fs.mkdir(path.dirname(authStorePath), { recursive: true }); + + return { + rootDir, + stateDir, + configPath, + authStorePath, + authJsonPath, + envPath, + env: { OPENCLAW_STATE_DIR: stateDir, OPENCLAW_CONFIG_PATH: configPath, OPENAI_API_KEY: "env-openai-key", - ...(typeof process.env.PATH === "string" && process.env.PATH.trim().length > 0 - ? { PATH: process.env.PATH } - : { PATH: "/usr/bin:/bin" }), - }; + PATH: resolveRuntimePathEnv(), + }, + }; +} - await fs.mkdir(path.dirname(configPath), { recursive: true }); - await fs.mkdir(path.dirname(authStorePath), { recursive: true }); - await fs.writeFile( - configPath, - `${JSON.stringify( - { - models: { - providers: { - openai: { - baseUrl: "https://api.openai.com/v1", - api: "openai-completions", - apiKey: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, - models: [{ id: "gpt-5", name: "gpt-5" }], - }, - }, - }, - }, - null, - 2, - )}\n`, - "utf8", - ); - await fs.writeFile( - authStorePath, - `${JSON.stringify( - { - version: 1, - profiles: { - "openai:default": { - type: "api_key", - provider: "openai", - key: "sk-openai-plaintext", - }, - }, - }, - null, - 2, - )}\n`, - "utf8", - ); - await fs.writeFile(envPath, "OPENAI_API_KEY=sk-openai-plaintext\n", "utf8"); +async function seedAuditFixture(fixture: AuditFixture): Promise { + const seededProvider = { + openai: { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, + models: [{ id: "gpt-5", name: "gpt-5" }], + }, + }; + const seededProfiles = new Map>([ + [ + "openai:default", + { + type: "api_key", + provider: "openai", + key: "sk-openai-plaintext", + }, + ], + ]); + await writeJsonFile(fixture.configPath, { + models: { providers: seededProvider }, + }); + await writeJsonFile(fixture.authStorePath, { + version: 1, + profiles: Object.fromEntries(seededProfiles), + }); + await fs.writeFile(fixture.envPath, "OPENAI_API_KEY=sk-openai-plaintext\n", "utf8"); +} + +describe("secrets audit", () => { + let fixture: AuditFixture; + + beforeEach(async () => { + fixture = await createAuditFixture(); + await seedAuditFixture(fixture); }); afterEach(async () => { - await fs.rm(rootDir, { recursive: true, force: true }); + await fs.rm(fixture.rootDir, { recursive: true, force: true }); }); it("reports plaintext + shadowing findings", async () => { - const report = await runSecretsAudit({ env }); + const report = await runSecretsAudit({ env: fixture.env }); expect(report.status).toBe("findings"); expect(report.summary.plaintextCount).toBeGreaterThan(0); expect(report.summary.shadowedRefCount).toBeGreaterThan(0); - expect(report.findings.some((entry) => entry.code === "REF_SHADOWED")).toBe(true); - expect(report.findings.some((entry) => entry.code === "PLAINTEXT_FOUND")).toBe(true); + expect(hasFinding(report, (entry) => entry.code === "REF_SHADOWED")).toBe(true); + expect(hasFinding(report, (entry) => entry.code === "PLAINTEXT_FOUND")).toBe(true); }); it("does not mutate legacy auth.json during audit", async () => { - await fs.rm(authStorePath, { force: true }); - await fs.writeFile( - authJsonPath, - `${JSON.stringify( - { - openai: { - type: "api_key", - key: "sk-legacy-auth-json", - }, - }, - null, - 2, - )}\n`, - "utf8", - ); + await fs.rm(fixture.authStorePath, { force: true }); + await writeJsonFile(fixture.authJsonPath, { + openai: { + type: "api_key", + key: "sk-legacy-auth-json", + }, + }); - const report = await runSecretsAudit({ env }); - expect(report.findings.some((entry) => entry.code === "LEGACY_RESIDUE")).toBe(true); - await expect(fs.stat(authJsonPath)).resolves.toBeTruthy(); - await expect(fs.stat(authStorePath)).rejects.toMatchObject({ code: "ENOENT" }); + const report = await runSecretsAudit({ env: fixture.env }); + expect(hasFinding(report, (entry) => entry.code === "LEGACY_RESIDUE")).toBe(true); + await expect(fs.stat(fixture.authJsonPath)).resolves.toBeTruthy(); + await expect(fs.stat(fixture.authStorePath)).rejects.toMatchObject({ code: "ENOENT" }); }); it("reports malformed sidecar JSON as findings instead of crashing", async () => { - await fs.writeFile(authStorePath, "{invalid-json", "utf8"); - await fs.writeFile(authJsonPath, "{invalid-json", "utf8"); + await fs.writeFile(fixture.authStorePath, "{invalid-json", "utf8"); + await fs.writeFile(fixture.authJsonPath, "{invalid-json", "utf8"); - const report = await runSecretsAudit({ env }); - expect(report.findings.some((entry) => entry.file === authStorePath)).toBe(true); - expect(report.findings.some((entry) => entry.file === authJsonPath)).toBe(true); - expect(report.findings.some((entry) => entry.code === "REF_UNRESOLVED")).toBe(true); + const report = await runSecretsAudit({ env: fixture.env }); + expect(hasFinding(report, (entry) => entry.file === fixture.authStorePath)).toBe(true); + expect(hasFinding(report, (entry) => entry.file === fixture.authJsonPath)).toBe(true); + expect(hasFinding(report, (entry) => entry.code === "REF_UNRESOLVED")).toBe(true); }); it("batches ref resolution per provider during audit", async () => { if (process.platform === "win32") { return; } - const execLogPath = path.join(rootDir, "exec-calls.log"); - const execScriptPath = path.join(rootDir, "resolver.mjs"); + const execLogPath = path.join(fixture.rootDir, "exec-calls.log"); + const execScriptPath = path.join(fixture.rootDir, "resolver.mjs"); await fs.writeFile( execScriptPath, [ @@ -137,47 +153,39 @@ describe("secrets audit", () => { { encoding: "utf8", mode: 0o700 }, ); - await fs.writeFile( - configPath, - `${JSON.stringify( - { - secrets: { - providers: { - execmain: { - source: "exec", - command: execScriptPath, - jsonOnly: true, - timeoutMs: 20_000, - noOutputTimeoutMs: 10_000, - }, - }, - }, - models: { - providers: { - openai: { - baseUrl: "https://api.openai.com/v1", - api: "openai-completions", - apiKey: { source: "exec", provider: "execmain", id: "providers/openai/apiKey" }, - models: [{ id: "gpt-5", name: "gpt-5" }], - }, - moonshot: { - baseUrl: "https://api.moonshot.cn/v1", - api: "openai-completions", - apiKey: { source: "exec", provider: "execmain", id: "providers/moonshot/apiKey" }, - models: [{ id: "moonshot-v1-8k", name: "moonshot-v1-8k" }], - }, - }, + await writeJsonFile(fixture.configPath, { + secrets: { + providers: { + execmain: { + source: "exec", + command: execScriptPath, + jsonOnly: true, + timeoutMs: 20_000, + noOutputTimeoutMs: 10_000, }, }, - null, - 2, - )}\n`, - "utf8", - ); - await fs.rm(authStorePath, { force: true }); - await fs.writeFile(envPath, "", "utf8"); + }, + models: { + providers: { + openai: { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey: { source: "exec", provider: "execmain", id: "providers/openai/apiKey" }, + models: [{ id: "gpt-5", name: "gpt-5" }], + }, + moonshot: { + baseUrl: "https://api.moonshot.cn/v1", + api: "openai-completions", + apiKey: { source: "exec", provider: "execmain", id: "providers/moonshot/apiKey" }, + models: [{ id: "moonshot-v1-8k", name: "moonshot-v1-8k" }], + }, + }, + }, + }); + await fs.rm(fixture.authStorePath, { force: true }); + await fs.writeFile(fixture.envPath, "", "utf8"); - const report = await runSecretsAudit({ env }); + const report = await runSecretsAudit({ env: fixture.env }); expect(report.summary.unresolvedRefCount).toBe(0); const callLog = await fs.readFile(execLogPath, "utf8"); diff --git a/src/secrets/resolve.test.ts b/src/secrets/resolve.test.ts index 62769bcb927..659124611a3 100644 --- a/src/secrets/resolve.test.ts +++ b/src/secrets/resolve.test.ts @@ -1,7 +1,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import { resolveSecretRefString, resolveSecretRefValue } from "./resolve.js"; @@ -12,28 +12,92 @@ async function writeSecureFile(filePath: string, content: string, mode = 0o600): } describe("secret ref resolver", () => { - let fixtureRoot = ""; - let caseId = 0; + const cleanupRoots: string[] = []; + const execRef = { source: "exec", provider: "execmain", id: "openai/api-key" } as const; + const fileRef = { source: "file", provider: "filemain", id: "/providers/openai/apiKey" } as const; - const createCaseDir = async (label: string): Promise => { - const dir = path.join(fixtureRoot, `${label}-${caseId++}`); - await fs.mkdir(dir, { recursive: true }); - return dir; - }; + function isWindows(): boolean { + return process.platform === "win32"; + } - beforeAll(async () => { - fixtureRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-secrets-resolve-")); - }); + async function createTempRoot(prefix: string): Promise { + const root = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + cleanupRoots.push(root); + return root; + } + + function createProviderConfig( + providerId: string, + provider: Record, + ): OpenClawConfig { + return { + secrets: { + providers: { + [providerId]: provider, + }, + }, + }; + } + + async function resolveWithProvider(params: { + ref: Parameters[0]; + providerId: string; + provider: Record; + }) { + return await resolveSecretRefString(params.ref, { + config: createProviderConfig(params.providerId, params.provider), + }); + } + + function createExecProvider( + command: string, + overrides?: Record, + ): Record { + return { + source: "exec", + command, + passEnv: ["PATH"], + ...overrides, + }; + } + + async function expectExecResolveRejects( + provider: Record, + message: string, + ): Promise { + await expect( + resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider, + }), + ).rejects.toThrow(message); + } + + async function createSymlinkedPlainExecCommand( + root: string, + targetRoot = root, + ): Promise<{ scriptPath: string; symlinkPath: string }> { + const scriptPath = path.join(targetRoot, "resolver-target.mjs"); + const symlinkPath = path.join(root, "resolver-link.mjs"); + await writeSecureFile( + scriptPath, + ["#!/usr/bin/env node", "process.stdout.write('plain-secret');"].join("\n"), + 0o700, + ); + await fs.symlink(scriptPath, symlinkPath); + return { scriptPath, symlinkPath }; + } afterEach(async () => { vi.restoreAllMocks(); - }); - - afterAll(async () => { - if (!fixtureRoot) { - return; + while (cleanupRoots.length > 0) { + const root = cleanupRoots.pop(); + if (!root) { + continue; + } + await fs.rm(root, { recursive: true, force: true }); } - await fs.rm(fixtureRoot, { recursive: true, force: true }); }); it("resolves env refs via implicit default env provider", async () => { @@ -49,10 +113,10 @@ describe("secret ref resolver", () => { }); it("resolves file refs in json mode", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("file"); + const root = await createTempRoot("openclaw-secrets-resolve-file-"); const filePath = path.join(root, "secrets.json"); await writeSecureFile( filePath, @@ -65,30 +129,23 @@ describe("secret ref resolver", () => { }), ); - const value = await resolveSecretRefString( - { source: "file", provider: "filemain", id: "/providers/openai/apiKey" }, - { - config: { - secrets: { - providers: { - filemain: { - source: "file", - path: filePath, - mode: "json", - }, - }, - }, - }, + const value = await resolveWithProvider({ + ref: fileRef, + providerId: "filemain", + provider: { + source: "file", + path: filePath, + mode: "json", }, - ); + }); expect(value).toBe("sk-file-value"); }); it("resolves exec refs with protocolVersion 1 response", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec"); + const root = await createTempRoot("openclaw-secrets-resolve-exec-"); const scriptPath = path.join(root, "resolver.mjs"); await writeSecureFile( scriptPath, @@ -102,30 +159,23 @@ describe("secret ref resolver", () => { 0o700, ); - const value = await resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: scriptPath, - passEnv: ["PATH"], - }, - }, - }, - }, + const value = await resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider: { + source: "exec", + command: scriptPath, + passEnv: ["PATH"], }, - ); + }); expect(value).toBe("value:openai/api-key"); }); it("supports non-JSON single-value exec output when jsonOnly is false", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-plain"); + const root = await createTempRoot("openclaw-secrets-resolve-exec-plain-"); const scriptPath = path.join(root, "resolver-plain.mjs"); await writeSecureFile( scriptPath, @@ -133,104 +183,57 @@ describe("secret ref resolver", () => { 0o700, ); - const value = await resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: scriptPath, - passEnv: ["PATH"], - jsonOnly: false, - }, - }, - }, - }, + const value = await resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider: { + source: "exec", + command: scriptPath, + passEnv: ["PATH"], + jsonOnly: false, }, - ); + }); expect(value).toBe("plain-secret"); }); it("rejects symlink command paths unless allowSymlinkCommand is enabled", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-link-reject"); - const scriptPath = path.join(root, "resolver-target.mjs"); - const symlinkPath = path.join(root, "resolver-link.mjs"); - await writeSecureFile( - scriptPath, - ["#!/usr/bin/env node", "process.stdout.write('plain-secret');"].join("\n"), - 0o700, + const root = await createTempRoot("openclaw-secrets-resolve-exec-link-"); + const { symlinkPath } = await createSymlinkedPlainExecCommand(root); + await expectExecResolveRejects( + createExecProvider(symlinkPath, { jsonOnly: false }), + "must not be a symlink", ); - await fs.symlink(scriptPath, symlinkPath); - - await expect( - resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: symlinkPath, - passEnv: ["PATH"], - jsonOnly: false, - }, - }, - }, - }, - }, - ), - ).rejects.toThrow("must not be a symlink"); }); it("allows symlink command paths when allowSymlinkCommand is enabled", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-link-allow"); - const scriptPath = path.join(root, "resolver-target.mjs"); - const symlinkPath = path.join(root, "resolver-link.mjs"); - await writeSecureFile( - scriptPath, - ["#!/usr/bin/env node", "process.stdout.write('plain-secret');"].join("\n"), - 0o700, - ); - await fs.symlink(scriptPath, symlinkPath); + const root = await createTempRoot("openclaw-secrets-resolve-exec-link-"); + const { symlinkPath } = await createSymlinkedPlainExecCommand(root); const trustedRoot = await fs.realpath(root); - const value = await resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: symlinkPath, - passEnv: ["PATH"], - jsonOnly: false, - allowSymlinkCommand: true, - trustedDirs: [trustedRoot], - }, - }, - }, - }, - }, - ); + const value = await resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider: createExecProvider(symlinkPath, { + jsonOnly: false, + allowSymlinkCommand: true, + trustedDirs: [trustedRoot], + }), + }); expect(value).toBe("plain-secret"); }); it("handles Homebrew-style symlinked exec commands with args only when explicitly allowed", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("homebrew"); + const root = await createTempRoot("openclaw-secrets-resolve-homebrew-"); const binDir = path.join(root, "opt", "homebrew", "bin"); const cellarDir = path.join(root, "opt", "homebrew", "Cellar", "node", "25.0.0", "bin"); await fs.mkdir(binDir, { recursive: true }); @@ -254,89 +257,54 @@ describe("secret ref resolver", () => { const trustedRoot = await fs.realpath(root); await expect( - resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: symlinkCommand, - args: ["brew"], - passEnv: ["PATH"], - }, - }, - }, - }, + resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider: { + source: "exec", + command: symlinkCommand, + args: ["brew"], + passEnv: ["PATH"], }, - ), + }), ).rejects.toThrow("must not be a symlink"); - const value = await resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: symlinkCommand, - args: ["brew"], - allowSymlinkCommand: true, - trustedDirs: [trustedRoot], - }, - }, - }, - }, + const value = await resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider: { + source: "exec", + command: symlinkCommand, + args: ["brew"], + allowSymlinkCommand: true, + trustedDirs: [trustedRoot], }, - ); + }); expect(value).toBe("brew:openai/api-key"); }); it("checks trustedDirs against resolved symlink target", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-link-trusted"); - const outside = await createCaseDir("exec-outside"); - const scriptPath = path.join(outside, "resolver-target.mjs"); - const symlinkPath = path.join(root, "resolver-link.mjs"); - await writeSecureFile( - scriptPath, - ["#!/usr/bin/env node", "process.stdout.write('plain-secret');"].join("\n"), - 0o700, + const root = await createTempRoot("openclaw-secrets-resolve-exec-link-"); + const outside = await createTempRoot("openclaw-secrets-resolve-exec-out-"); + const { symlinkPath } = await createSymlinkedPlainExecCommand(root, outside); + await expectExecResolveRejects( + createExecProvider(symlinkPath, { + jsonOnly: false, + allowSymlinkCommand: true, + trustedDirs: [root], + }), + "outside trustedDirs", ); - await fs.symlink(scriptPath, symlinkPath); - - await expect( - resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: symlinkPath, - passEnv: ["PATH"], - jsonOnly: false, - allowSymlinkCommand: true, - trustedDirs: [root], - }, - }, - }, - }, - }, - ), - ).rejects.toThrow("outside trustedDirs"); }); it("rejects exec refs when protocolVersion is not 1", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-protocol"); + const root = await createTempRoot("openclaw-secrets-resolve-exec-protocol-"); const scriptPath = path.join(root, "resolver-protocol.mjs"); await writeSecureFile( scriptPath, @@ -347,31 +315,14 @@ describe("secret ref resolver", () => { 0o700, ); - await expect( - resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: scriptPath, - passEnv: ["PATH"], - }, - }, - }, - }, - }, - ), - ).rejects.toThrow("protocolVersion must be 1"); + await expectExecResolveRejects(createExecProvider(scriptPath), "protocolVersion must be 1"); }); it("rejects exec refs when response omits requested id", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-missing-id"); + const root = await createTempRoot("openclaw-secrets-resolve-exec-id-"); const scriptPath = path.join(root, "resolver-missing-id.mjs"); await writeSecureFile( scriptPath, @@ -382,31 +333,17 @@ describe("secret ref resolver", () => { 0o700, ); - await expect( - resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: scriptPath, - passEnv: ["PATH"], - }, - }, - }, - }, - }, - ), - ).rejects.toThrow('response missing id "openai/api-key"'); + await expectExecResolveRejects( + createExecProvider(scriptPath), + 'response missing id "openai/api-key"', + ); }); it("rejects exec refs with invalid JSON when jsonOnly is true", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("exec-invalid-json"); + const root = await createTempRoot("openclaw-secrets-resolve-exec-json-"); const scriptPath = path.join(root, "resolver-invalid-json.mjs"); await writeSecureFile( scriptPath, @@ -415,58 +352,44 @@ describe("secret ref resolver", () => { ); await expect( - resolveSecretRefString( - { source: "exec", provider: "execmain", id: "openai/api-key" }, - { - config: { - secrets: { - providers: { - execmain: { - source: "exec", - command: scriptPath, - passEnv: ["PATH"], - jsonOnly: true, - }, - }, - }, - }, + resolveWithProvider({ + ref: execRef, + providerId: "execmain", + provider: { + source: "exec", + command: scriptPath, + passEnv: ["PATH"], + jsonOnly: true, }, - ), + }), ).rejects.toThrow("returned invalid JSON"); }); it("supports file singleValue mode with id=value", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("file-single-value"); + const root = await createTempRoot("openclaw-secrets-resolve-single-value-"); const filePath = path.join(root, "token.txt"); await writeSecureFile(filePath, "raw-token-value\n"); - const value = await resolveSecretRefString( - { source: "file", provider: "rawfile", id: "value" }, - { - config: { - secrets: { - providers: { - rawfile: { - source: "file", - path: filePath, - mode: "singleValue", - }, - }, - }, - }, + const value = await resolveWithProvider({ + ref: { source: "file", provider: "rawfile", id: "value" }, + providerId: "rawfile", + provider: { + source: "file", + path: filePath, + mode: "singleValue", }, - ); + }); expect(value).toBe("raw-token-value"); }); it("times out file provider reads when timeoutMs elapses", async () => { - if (process.platform === "win32") { + if (isWindows()) { return; } - const root = await createCaseDir("file-timeout"); + const root = await createTempRoot("openclaw-secrets-resolve-timeout-"); const filePath = path.join(root, "secrets.json"); await writeSecureFile( filePath, @@ -491,23 +414,16 @@ describe("secret ref resolver", () => { }) as typeof fs.readFile); await expect( - resolveSecretRefString( - { source: "file", provider: "filemain", id: "/providers/openai/apiKey" }, - { - config: { - secrets: { - providers: { - filemain: { - source: "file", - path: filePath, - mode: "json", - timeoutMs: 5, - }, - }, - }, - }, + resolveWithProvider({ + ref: fileRef, + providerId: "filemain", + provider: { + source: "file", + path: filePath, + mode: "json", + timeoutMs: 5, }, - ), + }), ).rejects.toThrow('File provider "filemain" timed out'); }); @@ -516,15 +432,7 @@ describe("secret ref resolver", () => { resolveSecretRefValue( { source: "exec", provider: "default", id: "abc" }, { - config: { - secrets: { - providers: { - default: { - source: "env", - }, - }, - }, - }, + config: createProviderConfig("default", { source: "env" }), }, ), ).rejects.toThrow('has source "env" but ref requests "exec"'); diff --git a/src/sessions/model-overrides.test.ts b/src/sessions/model-overrides.test.ts index 7e5d1b0b117..cdfe154b2c4 100644 --- a/src/sessions/model-overrides.test.ts +++ b/src/sessions/model-overrides.test.ts @@ -2,6 +2,24 @@ import { describe, expect, it } from "vitest"; import type { SessionEntry } from "../config/sessions.js"; import { applyModelOverrideToSessionEntry } from "./model-overrides.js"; +function applyOpenAiSelection(entry: SessionEntry) { + return applyModelOverrideToSessionEntry({ + entry, + selection: { + provider: "openai", + model: "gpt-5.2", + }, + }); +} + +function expectRuntimeModelFieldsCleared(entry: SessionEntry, before: number) { + expect(entry.providerOverride).toBe("openai"); + expect(entry.modelOverride).toBe("gpt-5.2"); + expect(entry.modelProvider).toBeUndefined(); + expect(entry.model).toBeUndefined(); + expect((entry.updatedAt ?? 0) > before).toBe(true); +} + describe("applyModelOverrideToSessionEntry", () => { it("clears stale runtime model fields when switching overrides", () => { const before = Date.now() - 5_000; @@ -17,23 +35,13 @@ describe("applyModelOverrideToSessionEntry", () => { fallbackNoticeReason: "provider temporary failure", }; - const result = applyModelOverrideToSessionEntry({ - entry, - selection: { - provider: "openai", - model: "gpt-5.2", - }, - }); + const result = applyOpenAiSelection(entry); expect(result.updated).toBe(true); - expect(entry.providerOverride).toBe("openai"); - expect(entry.modelOverride).toBe("gpt-5.2"); - expect(entry.modelProvider).toBeUndefined(); - expect(entry.model).toBeUndefined(); + expectRuntimeModelFieldsCleared(entry, before); expect(entry.fallbackNoticeSelectedModel).toBeUndefined(); expect(entry.fallbackNoticeActiveModel).toBeUndefined(); expect(entry.fallbackNoticeReason).toBeUndefined(); - expect((entry.updatedAt ?? 0) > before).toBe(true); }); it("clears stale runtime model fields even when override selection is unchanged", () => { @@ -47,20 +55,10 @@ describe("applyModelOverrideToSessionEntry", () => { modelOverride: "gpt-5.2", }; - const result = applyModelOverrideToSessionEntry({ - entry, - selection: { - provider: "openai", - model: "gpt-5.2", - }, - }); + const result = applyOpenAiSelection(entry); expect(result.updated).toBe(true); - expect(entry.providerOverride).toBe("openai"); - expect(entry.modelOverride).toBe("gpt-5.2"); - expect(entry.modelProvider).toBeUndefined(); - expect(entry.model).toBeUndefined(); - expect((entry.updatedAt ?? 0) > before).toBe(true); + expectRuntimeModelFieldsCleared(entry, before); }); it("retains aligned runtime model fields when selection and runtime already match", () => { diff --git a/src/slack/actions.download-file.test.ts b/src/slack/actions.download-file.test.ts index b7afe84b149..d75330435ad 100644 --- a/src/slack/actions.download-file.test.ts +++ b/src/slack/actions.download-file.test.ts @@ -21,6 +21,45 @@ function createClient() { }; } +function makeSlackFileInfo(overrides?: Record) { + return { + id: "F123", + name: "image.png", + mimetype: "image/png", + url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", + ...overrides, + }; +} + +function makeResolvedSlackMedia() { + return { + path: "/tmp/image.png", + contentType: "image/png", + placeholder: "[Slack file: image.png]", + }; +} + +function expectNoMediaDownload(result: Awaited>) { + expect(result).toBeNull(); + expect(resolveSlackMedia).not.toHaveBeenCalled(); +} + +function expectResolveSlackMediaCalledWithDefaults() { + expect(resolveSlackMedia).toHaveBeenCalledWith({ + files: [ + { + id: "F123", + name: "image.png", + mimetype: "image/png", + url_private: undefined, + url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", + }, + ], + token: "xoxb-test", + maxBytes: 1024, + }); +} + describe("downloadSlackFile", () => { beforeEach(() => { resolveSlackMedia.mockReset(); @@ -48,20 +87,9 @@ describe("downloadSlackFile", () => { it("downloads via resolveSlackMedia using fresh files.info metadata", async () => { const client = createClient(); client.files.info.mockResolvedValueOnce({ - file: { - id: "F123", - name: "image.png", - mimetype: "image/png", - url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", - }, + file: makeSlackFileInfo(), }); - resolveSlackMedia.mockResolvedValueOnce([ - { - path: "/tmp/image.png", - contentType: "image/png", - placeholder: "[Slack file: image.png]", - }, - ]); + resolveSlackMedia.mockResolvedValueOnce([makeResolvedSlackMedia()]); const result = await downloadSlackFile("F123", { client, @@ -70,36 +98,14 @@ describe("downloadSlackFile", () => { }); expect(client.files.info).toHaveBeenCalledWith({ file: "F123" }); - expect(resolveSlackMedia).toHaveBeenCalledWith({ - files: [ - { - id: "F123", - name: "image.png", - mimetype: "image/png", - url_private: undefined, - url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", - }, - ], - token: "xoxb-test", - maxBytes: 1024, - }); - expect(result).toEqual({ - path: "/tmp/image.png", - contentType: "image/png", - placeholder: "[Slack file: image.png]", - }); + expectResolveSlackMediaCalledWithDefaults(); + expect(result).toEqual(makeResolvedSlackMedia()); }); it("returns null when channel scope definitely mismatches file shares", async () => { const client = createClient(); client.files.info.mockResolvedValueOnce({ - file: { - id: "F123", - name: "image.png", - mimetype: "image/png", - url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", - channels: ["C999"], - }, + file: makeSlackFileInfo({ channels: ["C999"] }), }); const result = await downloadSlackFile("F123", { @@ -109,24 +115,19 @@ describe("downloadSlackFile", () => { channelId: "C123", }); - expect(result).toBeNull(); - expect(resolveSlackMedia).not.toHaveBeenCalled(); + expectNoMediaDownload(result); }); it("returns null when thread scope definitely mismatches file share thread", async () => { const client = createClient(); client.files.info.mockResolvedValueOnce({ - file: { - id: "F123", - name: "image.png", - mimetype: "image/png", - url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", + file: makeSlackFileInfo({ shares: { private: { C123: [{ ts: "111.111", thread_ts: "111.111" }], }, }, - }, + }), }); const result = await downloadSlackFile("F123", { @@ -137,27 +138,15 @@ describe("downloadSlackFile", () => { threadId: "222.222", }); - expect(result).toBeNull(); - expect(resolveSlackMedia).not.toHaveBeenCalled(); + expectNoMediaDownload(result); }); it("keeps legacy behavior when file metadata does not expose channel/thread shares", async () => { const client = createClient(); client.files.info.mockResolvedValueOnce({ - file: { - id: "F123", - name: "image.png", - mimetype: "image/png", - url_private_download: "https://files.slack.com/files-pri/T1-F123/image.png", - }, + file: makeSlackFileInfo(), }); - resolveSlackMedia.mockResolvedValueOnce([ - { - path: "/tmp/image.png", - contentType: "image/png", - placeholder: "[Slack file: image.png]", - }, - ]); + resolveSlackMedia.mockResolvedValueOnce([makeResolvedSlackMedia()]); const result = await downloadSlackFile("F123", { client, @@ -167,11 +156,8 @@ describe("downloadSlackFile", () => { threadId: "222.222", }); - expect(result).toEqual({ - path: "/tmp/image.png", - contentType: "image/png", - placeholder: "[Slack file: image.png]", - }); + expect(result).toEqual(makeResolvedSlackMedia()); expect(resolveSlackMedia).toHaveBeenCalledTimes(1); + expectResolveSlackMediaCalledWithDefaults(); }); }); diff --git a/src/slack/monitor/events/members.test.ts b/src/slack/monitor/events/members.test.ts index d476a492e6e..93f8727d5f2 100644 --- a/src/slack/monitor/events/members.test.ts +++ b/src/slack/monitor/events/members.test.ts @@ -1,44 +1,35 @@ import { describe, expect, it, vi } from "vitest"; import { registerSlackMemberEvents } from "./members.js"; import { - createSlackSystemEventTestHarness, - type SlackSystemEventTestOverrides, + createSlackSystemEventTestHarness as initSlackHarness, + type SlackSystemEventTestOverrides as MemberOverrides, } from "./system-event-test-harness.js"; -const enqueueSystemEventMock = vi.fn(); -const readAllowFromStoreMock = vi.fn(); +const memberMocks = vi.hoisted(() => ({ + enqueue: vi.fn(), + readAllow: vi.fn(), +})); vi.mock("../../../infra/system-events.js", () => ({ - enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args), + enqueueSystemEvent: memberMocks.enqueue, })); vi.mock("../../../pairing/pairing-store.js", () => ({ - readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args), + readChannelAllowFromStore: memberMocks.readAllow, })); -type SlackMemberHandler = (args: { - event: Record; - body: unknown; -}) => Promise; +type MemberHandler = (args: { event: Record; body: unknown }) => Promise; -function createMembersContext(params?: { - overrides?: SlackSystemEventTestOverrides; +type MemberCaseArgs = { + event?: Record; + body?: unknown; + overrides?: MemberOverrides; + handler?: "joined" | "left"; trackEvent?: () => void; shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; -}) { - const harness = createSlackSystemEventTestHarness(params?.overrides); - if (params?.shouldDropMismatchedSlackEvent) { - harness.ctx.shouldDropMismatchedSlackEvent = params.shouldDropMismatchedSlackEvent; - } - registerSlackMemberEvents({ ctx: harness.ctx, trackEvent: params?.trackEvent }); - return { - getJoinedHandler: () => - harness.getHandler("member_joined_channel") as SlackMemberHandler | null, - getLeftHandler: () => harness.getHandler("member_left_channel") as SlackMemberHandler | null, - }; -} +}; -function makeMemberEvent(overrides?: { user?: string; channel?: string }) { +function makeMemberEvent(overrides?: { channel?: string; user?: string }) { return { type: "member_joined_channel", user: overrides?.user ?? "U1", @@ -47,106 +38,90 @@ function makeMemberEvent(overrides?: { user?: string; channel?: string }) { }; } +function getMemberHandlers(params: { + overrides?: MemberOverrides; + trackEvent?: () => void; + shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; +}) { + const harness = initSlackHarness(params.overrides); + if (params.shouldDropMismatchedSlackEvent) { + harness.ctx.shouldDropMismatchedSlackEvent = params.shouldDropMismatchedSlackEvent; + } + registerSlackMemberEvents({ ctx: harness.ctx, trackEvent: params.trackEvent }); + return { + joined: harness.getHandler("member_joined_channel") as MemberHandler | null, + left: harness.getHandler("member_left_channel") as MemberHandler | null, + }; +} + +async function runMemberCase(args: MemberCaseArgs = {}): Promise { + memberMocks.enqueue.mockClear(); + memberMocks.readAllow.mockReset().mockResolvedValue([]); + const handlers = getMemberHandlers({ + overrides: args.overrides, + trackEvent: args.trackEvent, + shouldDropMismatchedSlackEvent: args.shouldDropMismatchedSlackEvent, + }); + const key = args.handler ?? "joined"; + const handler = handlers[key]; + expect(handler).toBeTruthy(); + await handler!({ + event: (args.event ?? makeMemberEvent()) as Record, + body: args.body ?? {}, + }); +} + describe("registerSlackMemberEvents", () => { - it("enqueues DM member events when dmPolicy is open", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getJoinedHandler } = createMembersContext({ overrides: { dmPolicy: "open" } }); - const joinedHandler = getJoinedHandler(); - expect(joinedHandler).toBeTruthy(); - - await joinedHandler!({ - event: makeMemberEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks DM member events when dmPolicy is disabled", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getJoinedHandler } = createMembersContext({ overrides: { dmPolicy: "disabled" } }); - const joinedHandler = getJoinedHandler(); - expect(joinedHandler).toBeTruthy(); - - await joinedHandler!({ - event: makeMemberEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("blocks DM member events for unauthorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getJoinedHandler } = createMembersContext({ - overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, - }); - const joinedHandler = getJoinedHandler(); - expect(joinedHandler).toBeTruthy(); - - await joinedHandler!({ - event: makeMemberEvent({ user: "U1" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("allows DM member events for authorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getLeftHandler } = createMembersContext({ - overrides: { dmPolicy: "allowlist", allowFrom: ["U1"] }, - }); - const leftHandler = getLeftHandler(); - expect(leftHandler).toBeTruthy(); - - await leftHandler!({ - event: { - ...makeMemberEvent({ user: "U1" }), - type: "member_left_channel", + it.each([ + { + name: "enqueues DM member events when dmPolicy is open", + args: { overrides: { dmPolicy: "open" } }, + calls: 1, + }, + { + name: "blocks DM member events when dmPolicy is disabled", + args: { overrides: { dmPolicy: "disabled" } }, + calls: 0, + }, + { + name: "blocks DM member events for unauthorized senders in allowlist mode", + args: { + overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, + event: makeMemberEvent({ user: "U1" }), }, - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks channel member events for users outside channel users allowlist", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getJoinedHandler } = createMembersContext({ - overrides: { - dmPolicy: "open", - channelType: "channel", - channelUsers: ["U_OWNER"], + calls: 0, + }, + { + name: "allows DM member events for authorized senders in allowlist mode", + args: { + handler: "left" as const, + overrides: { dmPolicy: "allowlist", allowFrom: ["U1"] }, + event: { ...makeMemberEvent({ user: "U1" }), type: "member_left_channel" }, }, - }); - const joinedHandler = getJoinedHandler(); - expect(joinedHandler).toBeTruthy(); - - await joinedHandler!({ - event: makeMemberEvent({ channel: "C1", user: "U_ATTACKER" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + calls: 1, + }, + { + name: "blocks channel member events for users outside channel users allowlist", + args: { + overrides: { + dmPolicy: "open", + channelType: "channel", + channelUsers: ["U_OWNER"], + }, + event: makeMemberEvent({ channel: "C1", user: "U_ATTACKER" }), + }, + calls: 0, + }, + ])("$name", async ({ args, calls }) => { + await runMemberCase(args); + expect(memberMocks.enqueue).toHaveBeenCalledTimes(calls); }); it("does not track mismatched events", async () => { const trackEvent = vi.fn(); - const { getJoinedHandler } = createMembersContext({ + await runMemberCase({ trackEvent, shouldDropMismatchedSlackEvent: () => true, - }); - const joinedHandler = getJoinedHandler(); - expect(joinedHandler).toBeTruthy(); - - await joinedHandler!({ - event: makeMemberEvent(), body: { api_app_id: "A_OTHER" }, }); @@ -155,14 +130,7 @@ describe("registerSlackMemberEvents", () => { it("tracks accepted member events", async () => { const trackEvent = vi.fn(); - const { getJoinedHandler } = createMembersContext({ trackEvent }); - const joinedHandler = getJoinedHandler(); - expect(joinedHandler).toBeTruthy(); - - await joinedHandler!({ - event: makeMemberEvent(), - body: {}, - }); + await runMemberCase({ trackEvent }); expect(trackEvent).toHaveBeenCalledTimes(1); }); diff --git a/src/slack/monitor/events/messages.test.ts b/src/slack/monitor/events/messages.test.ts index 0534cdcfa73..0e899a1fe71 100644 --- a/src/slack/monitor/events/messages.test.ts +++ b/src/slack/monitor/events/messages.test.ts @@ -5,23 +5,26 @@ import { type SlackSystemEventTestOverrides, } from "./system-event-test-harness.js"; -const enqueueSystemEventMock = vi.fn(); -const readAllowFromStoreMock = vi.fn(); +const messageQueueMock = vi.fn(); +const messageAllowMock = vi.fn(); vi.mock("../../../infra/system-events.js", () => ({ - enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args), + enqueueSystemEvent: (...args: unknown[]) => messageQueueMock(...args), })); vi.mock("../../../pairing/pairing-store.js", () => ({ - readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args), + readChannelAllowFromStore: (...args: unknown[]) => messageAllowMock(...args), })); -type SlackMessageHandler = (args: { - event: Record; - body: unknown; -}) => Promise; +type MessageHandler = (args: { event: Record; body: unknown }) => Promise; -function createMessagesContext(overrides?: SlackSystemEventTestOverrides) { +type MessageCase = { + overrides?: SlackSystemEventTestOverrides; + event?: Record; + body?: unknown; +}; + +function createMessageHandlers(overrides?: SlackSystemEventTestOverrides) { const harness = createSlackSystemEventTestHarness(overrides); const handleSlackMessage = vi.fn(async () => {}); registerSlackMessageEvents({ @@ -29,7 +32,7 @@ function createMessagesContext(overrides?: SlackSystemEventTestOverrides) { handleSlackMessage, }); return { - getMessageHandler: () => harness.getHandler("message") as SlackMessageHandler | null, + handler: harness.getHandler("message") as MessageHandler | null, handleSlackMessage, }; } @@ -40,14 +43,8 @@ function makeChangedEvent(overrides?: { channel?: string; user?: string }) { type: "message", subtype: "message_changed", channel: overrides?.channel ?? "D1", - message: { - ts: "123.456", - user, - }, - previous_message: { - ts: "123.450", - user, - }, + message: { ts: "123.456", user }, + previous_message: { ts: "123.450", user }, event_ts: "123.456", }; } @@ -73,113 +70,78 @@ function makeThreadBroadcastEvent(overrides?: { channel?: string; user?: string subtype: "thread_broadcast", channel: overrides?.channel ?? "D1", user, - message: { - ts: "123.456", - user, - }, + message: { ts: "123.456", user }, event_ts: "123.456", }; } +async function runMessageCase(input: MessageCase = {}): Promise { + messageQueueMock.mockClear(); + messageAllowMock.mockReset().mockResolvedValue([]); + const { handler } = createMessageHandlers(input.overrides); + expect(handler).toBeTruthy(); + await handler!({ + event: (input.event ?? makeChangedEvent()) as Record, + body: input.body ?? {}, + }); +} + describe("registerSlackMessageEvents", () => { - it("enqueues message_changed system events when dmPolicy is open", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getMessageHandler } = createMessagesContext({ dmPolicy: "open" }); - const messageHandler = getMessageHandler(); - expect(messageHandler).toBeTruthy(); - - await messageHandler!({ - event: makeChangedEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks message_changed system events when dmPolicy is disabled", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getMessageHandler } = createMessagesContext({ dmPolicy: "disabled" }); - const messageHandler = getMessageHandler(); - expect(messageHandler).toBeTruthy(); - - await messageHandler!({ - event: makeChangedEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("blocks message_changed system events for unauthorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getMessageHandler } = createMessagesContext({ - dmPolicy: "allowlist", - allowFrom: ["U2"], - }); - const messageHandler = getMessageHandler(); - expect(messageHandler).toBeTruthy(); - - await messageHandler!({ - event: makeChangedEvent({ user: "U1" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("blocks message_deleted system events for users outside channel users allowlist", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getMessageHandler } = createMessagesContext({ - dmPolicy: "open", - channelType: "channel", - channelUsers: ["U_OWNER"], - }); - const messageHandler = getMessageHandler(); - expect(messageHandler).toBeTruthy(); - - await messageHandler!({ - event: makeDeletedEvent({ channel: "C1", user: "U_ATTACKER" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("blocks thread_broadcast system events without an authenticated sender", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getMessageHandler } = createMessagesContext({ dmPolicy: "open" }); - const messageHandler = getMessageHandler(); - expect(messageHandler).toBeTruthy(); - - await messageHandler!({ - event: { - ...makeThreadBroadcastEvent(), - user: undefined, - message: { - ts: "123.456", + it.each([ + { + name: "enqueues message_changed system events when dmPolicy is open", + input: { overrides: { dmPolicy: "open" }, event: makeChangedEvent() }, + calls: 1, + }, + { + name: "blocks message_changed system events when dmPolicy is disabled", + input: { overrides: { dmPolicy: "disabled" }, event: makeChangedEvent() }, + calls: 0, + }, + { + name: "blocks message_changed system events for unauthorized senders in allowlist mode", + input: { + overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, + event: makeChangedEvent({ user: "U1" }), + }, + calls: 0, + }, + { + name: "blocks message_deleted system events for users outside channel users allowlist", + input: { + overrides: { + dmPolicy: "open", + channelType: "channel", + channelUsers: ["U_OWNER"], + }, + event: makeDeletedEvent({ channel: "C1", user: "U_ATTACKER" }), + }, + calls: 0, + }, + { + name: "blocks thread_broadcast system events without an authenticated sender", + input: { + overrides: { dmPolicy: "open" }, + event: { + ...makeThreadBroadcastEvent(), + user: undefined, + message: { ts: "123.456" }, }, }, - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + calls: 0, + }, + ])("$name", async ({ input, calls }) => { + await runMessageCase(input); + expect(messageQueueMock).toHaveBeenCalledTimes(calls); }); it("passes regular message events to the message handler", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getMessageHandler, handleSlackMessage } = createMessagesContext({ - dmPolicy: "open", - }); - const messageHandler = getMessageHandler(); - expect(messageHandler).toBeTruthy(); + messageQueueMock.mockClear(); + messageAllowMock.mockReset().mockResolvedValue([]); + const { handler, handleSlackMessage } = createMessageHandlers({ dmPolicy: "open" }); + expect(handler).toBeTruthy(); - await messageHandler!({ + await handler!({ event: { type: "message", channel: "D1", @@ -191,6 +153,6 @@ describe("registerSlackMessageEvents", () => { }); expect(handleSlackMessage).toHaveBeenCalledTimes(1); - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(messageQueueMock).not.toHaveBeenCalled(); }); }); diff --git a/src/slack/monitor/events/pins.test.ts b/src/slack/monitor/events/pins.test.ts index 17b5e50d62e..f71a8327e0f 100644 --- a/src/slack/monitor/events/pins.test.ts +++ b/src/slack/monitor/events/pins.test.ts @@ -1,40 +1,32 @@ import { describe, expect, it, vi } from "vitest"; import { registerSlackPinEvents } from "./pins.js"; import { - createSlackSystemEventTestHarness, - type SlackSystemEventTestOverrides, + createSlackSystemEventTestHarness as buildPinHarness, + type SlackSystemEventTestOverrides as PinOverrides, } from "./system-event-test-harness.js"; -const enqueueSystemEventMock = vi.fn(); -const readAllowFromStoreMock = vi.fn(); - -vi.mock("../../../infra/system-events.js", () => ({ - enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args), -})); +const pinEnqueueMock = vi.hoisted(() => vi.fn()); +const pinAllowMock = vi.hoisted(() => vi.fn()); +vi.mock("../../../infra/system-events.js", () => { + return { enqueueSystemEvent: pinEnqueueMock }; +}); vi.mock("../../../pairing/pairing-store.js", () => ({ - readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args), + readChannelAllowFromStore: pinAllowMock, })); -type SlackPinHandler = (args: { event: Record; body: unknown }) => Promise; +type PinHandler = (args: { event: Record; body: unknown }) => Promise; -function createPinContext(params?: { - overrides?: SlackSystemEventTestOverrides; +type PinCase = { + body?: unknown; + event?: Record; + handler?: "added" | "removed"; + overrides?: PinOverrides; trackEvent?: () => void; shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; -}) { - const harness = createSlackSystemEventTestHarness(params?.overrides); - if (params?.shouldDropMismatchedSlackEvent) { - harness.ctx.shouldDropMismatchedSlackEvent = params.shouldDropMismatchedSlackEvent; - } - registerSlackPinEvents({ ctx: harness.ctx, trackEvent: params?.trackEvent }); - return { - getAddedHandler: () => harness.getHandler("pin_added") as SlackPinHandler | null, - getRemovedHandler: () => harness.getHandler("pin_removed") as SlackPinHandler | null, - }; -} +}; -function makePinEvent(overrides?: { user?: string; channel?: string }) { +function makePinEvent(overrides?: { channel?: string; user?: string }) { return { type: "pin_added", user: overrides?.user ?? "U1", @@ -42,110 +34,92 @@ function makePinEvent(overrides?: { user?: string; channel?: string }) { event_ts: "123.456", item: { type: "message", - message: { - ts: "123.456", - }, + message: { ts: "123.456" }, }, }; } +function installPinHandlers(args: { + overrides?: PinOverrides; + trackEvent?: () => void; + shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; +}) { + const harness = buildPinHarness(args.overrides); + if (args.shouldDropMismatchedSlackEvent) { + harness.ctx.shouldDropMismatchedSlackEvent = args.shouldDropMismatchedSlackEvent; + } + registerSlackPinEvents({ ctx: harness.ctx, trackEvent: args.trackEvent }); + return { + added: harness.getHandler("pin_added") as PinHandler | null, + removed: harness.getHandler("pin_removed") as PinHandler | null, + }; +} + +async function runPinCase(input: PinCase = {}): Promise { + pinEnqueueMock.mockClear(); + pinAllowMock.mockReset().mockResolvedValue([]); + const { added, removed } = installPinHandlers({ + overrides: input.overrides, + trackEvent: input.trackEvent, + shouldDropMismatchedSlackEvent: input.shouldDropMismatchedSlackEvent, + }); + const handlerKey = input.handler ?? "added"; + const handler = handlerKey === "removed" ? removed : added; + expect(handler).toBeTruthy(); + const event = (input.event ?? makePinEvent()) as Record; + const body = input.body ?? {}; + await handler!({ + body, + event, + }); +} + describe("registerSlackPinEvents", () => { - it("enqueues DM pin system events when dmPolicy is open", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createPinContext({ overrides: { dmPolicy: "open" } }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks DM pin system events when dmPolicy is disabled", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createPinContext({ overrides: { dmPolicy: "disabled" } }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("blocks DM pin system events for unauthorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createPinContext({ - overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent({ user: "U1" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("allows DM pin system events for authorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createPinContext({ - overrides: { dmPolicy: "allowlist", allowFrom: ["U1"] }, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent({ user: "U1" }), - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks channel pin events for users outside channel users allowlist", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createPinContext({ - overrides: { - dmPolicy: "open", - channelType: "channel", - channelUsers: ["U_OWNER"], + it.each([ + ["enqueues DM pin system events when dmPolicy is open", { overrides: { dmPolicy: "open" } }, 1], + [ + "blocks DM pin system events when dmPolicy is disabled", + { overrides: { dmPolicy: "disabled" } }, + 0, + ], + [ + "blocks DM pin system events for unauthorized senders in allowlist mode", + { + overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, + event: makePinEvent({ user: "U1" }), }, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent({ channel: "C1", user: "U_ATTACKER" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + 0, + ], + [ + "allows DM pin system events for authorized senders in allowlist mode", + { + overrides: { dmPolicy: "allowlist", allowFrom: ["U1"] }, + event: makePinEvent({ user: "U1" }), + }, + 1, + ], + [ + "blocks channel pin events for users outside channel users allowlist", + { + overrides: { + dmPolicy: "open", + channelType: "channel", + channelUsers: ["U_OWNER"], + }, + event: makePinEvent({ channel: "C1", user: "U_ATTACKER" }), + }, + 0, + ], + ])("%s", async (_name, args: PinCase, expectedCalls: number) => { + await runPinCase(args); + expect(pinEnqueueMock).toHaveBeenCalledTimes(expectedCalls); }); it("does not track mismatched events", async () => { const trackEvent = vi.fn(); - const { getAddedHandler } = createPinContext({ + await runPinCase({ trackEvent, shouldDropMismatchedSlackEvent: () => true, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent(), body: { api_app_id: "A_OTHER" }, }); @@ -154,14 +128,7 @@ describe("registerSlackPinEvents", () => { it("tracks accepted pin events", async () => { const trackEvent = vi.fn(); - const { getAddedHandler } = createPinContext({ trackEvent }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makePinEvent(), - body: {}, - }); + await runPinCase({ trackEvent }); expect(trackEvent).toHaveBeenCalledTimes(1); }); diff --git a/src/slack/monitor/events/reactions.test.ts b/src/slack/monitor/events/reactions.test.ts index 84269c73e5d..656c9b0db8d 100644 --- a/src/slack/monitor/events/reactions.test.ts +++ b/src/slack/monitor/events/reactions.test.ts @@ -5,39 +5,33 @@ import { type SlackSystemEventTestOverrides, } from "./system-event-test-harness.js"; -const enqueueSystemEventMock = vi.fn(); -const readAllowFromStoreMock = vi.fn(); +const reactionQueueMock = vi.fn(); +const reactionAllowMock = vi.fn(); -vi.mock("../../../infra/system-events.js", () => ({ - enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args), -})); +vi.mock("../../../infra/system-events.js", () => { + return { + enqueueSystemEvent: (...args: unknown[]) => reactionQueueMock(...args), + }; +}); -vi.mock("../../../pairing/pairing-store.js", () => ({ - readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args), -})); +vi.mock("../../../pairing/pairing-store.js", () => { + return { + readChannelAllowFromStore: (...args: unknown[]) => reactionAllowMock(...args), + }; +}); -type SlackReactionHandler = (args: { - event: Record; - body: unknown; -}) => Promise; +type ReactionHandler = (args: { event: Record; body: unknown }) => Promise; -function createReactionContext(params?: { +type ReactionRunInput = { + handler?: "added" | "removed"; overrides?: SlackSystemEventTestOverrides; + event?: Record; + body?: unknown; trackEvent?: () => void; shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; -}) { - const harness = createSlackSystemEventTestHarness(params?.overrides); - if (params?.shouldDropMismatchedSlackEvent) { - harness.ctx.shouldDropMismatchedSlackEvent = params.shouldDropMismatchedSlackEvent; - } - registerSlackReactionEvents({ ctx: harness.ctx, trackEvent: params?.trackEvent }); - return { - getAddedHandler: () => harness.getHandler("reaction_added") as SlackReactionHandler | null, - getRemovedHandler: () => harness.getHandler("reaction_removed") as SlackReactionHandler | null, - }; -} +}; -function makeReactionEvent(overrides?: { user?: string; channel?: string }) { +function buildReactionEvent(overrides?: { user?: string; channel?: string }) { return { type: "reaction_added", user: overrides?.user ?? "U1", @@ -51,123 +45,100 @@ function makeReactionEvent(overrides?: { user?: string; channel?: string }) { }; } +function createReactionHandlers(params: { + overrides?: SlackSystemEventTestOverrides; + trackEvent?: () => void; + shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; +}) { + const harness = createSlackSystemEventTestHarness(params.overrides); + if (params.shouldDropMismatchedSlackEvent) { + harness.ctx.shouldDropMismatchedSlackEvent = params.shouldDropMismatchedSlackEvent; + } + registerSlackReactionEvents({ ctx: harness.ctx, trackEvent: params.trackEvent }); + return { + added: harness.getHandler("reaction_added") as ReactionHandler | null, + removed: harness.getHandler("reaction_removed") as ReactionHandler | null, + }; +} + +async function executeReactionCase(input: ReactionRunInput = {}) { + reactionQueueMock.mockClear(); + reactionAllowMock.mockReset().mockResolvedValue([]); + const handlers = createReactionHandlers({ + overrides: input.overrides, + trackEvent: input.trackEvent, + shouldDropMismatchedSlackEvent: input.shouldDropMismatchedSlackEvent, + }); + const handler = handlers[input.handler ?? "added"]; + expect(handler).toBeTruthy(); + await handler!({ + event: (input.event ?? buildReactionEvent()) as Record, + body: input.body ?? {}, + }); +} + describe("registerSlackReactionEvents", () => { - it("enqueues DM reaction system events when dmPolicy is open", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createReactionContext({ overrides: { dmPolicy: "open" } }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks DM reaction system events when dmPolicy is disabled", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createReactionContext({ overrides: { dmPolicy: "disabled" } }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent(), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("blocks DM reaction system events for unauthorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createReactionContext({ - overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent({ user: "U1" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); - }); - - it("allows DM reaction system events for authorized senders in allowlist mode", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createReactionContext({ - overrides: { dmPolicy: "allowlist", allowFrom: ["U1"] }, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent({ user: "U1" }), - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("enqueues channel reaction events regardless of dmPolicy", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getRemovedHandler } = createReactionContext({ - overrides: { dmPolicy: "disabled", channelType: "channel" }, - }); - const removedHandler = getRemovedHandler(); - expect(removedHandler).toBeTruthy(); - - await removedHandler!({ - event: { - ...makeReactionEvent({ channel: "C1" }), - type: "reaction_removed", + it.each([ + { + name: "enqueues DM reaction system events when dmPolicy is open", + args: { overrides: { dmPolicy: "open" } }, + expectedCalls: 1, + }, + { + name: "blocks DM reaction system events when dmPolicy is disabled", + args: { overrides: { dmPolicy: "disabled" } }, + expectedCalls: 0, + }, + { + name: "blocks DM reaction system events for unauthorized senders in allowlist mode", + args: { + overrides: { dmPolicy: "allowlist", allowFrom: ["U2"] }, + event: buildReactionEvent({ user: "U1" }), }, - body: {}, - }); - - expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); - }); - - it("blocks channel reaction events for users outside channel users allowlist", async () => { - enqueueSystemEventMock.mockClear(); - readAllowFromStoreMock.mockReset().mockResolvedValue([]); - const { getAddedHandler } = createReactionContext({ - overrides: { - dmPolicy: "open", - channelType: "channel", - channelUsers: ["U_OWNER"], + expectedCalls: 0, + }, + { + name: "allows DM reaction system events for authorized senders in allowlist mode", + args: { + overrides: { dmPolicy: "allowlist", allowFrom: ["U1"] }, + event: buildReactionEvent({ user: "U1" }), }, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent({ channel: "C1", user: "U_ATTACKER" }), - body: {}, - }); - - expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expectedCalls: 1, + }, + { + name: "enqueues channel reaction events regardless of dmPolicy", + args: { + handler: "removed" as const, + overrides: { dmPolicy: "disabled", channelType: "channel" }, + event: { + ...buildReactionEvent({ channel: "C1" }), + type: "reaction_removed", + }, + }, + expectedCalls: 1, + }, + { + name: "blocks channel reaction events for users outside channel users allowlist", + args: { + overrides: { + dmPolicy: "open", + channelType: "channel", + channelUsers: ["U_OWNER"], + }, + event: buildReactionEvent({ channel: "C1", user: "U_ATTACKER" }), + }, + expectedCalls: 0, + }, + ])("$name", async ({ args, expectedCalls }) => { + await executeReactionCase(args); + expect(reactionQueueMock).toHaveBeenCalledTimes(expectedCalls); }); it("does not track mismatched events", async () => { const trackEvent = vi.fn(); - const { getAddedHandler } = createReactionContext({ + await executeReactionCase({ trackEvent, shouldDropMismatchedSlackEvent: () => true, - }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent(), body: { api_app_id: "A_OTHER" }, }); @@ -176,14 +147,7 @@ describe("registerSlackReactionEvents", () => { it("tracks accepted message reactions", async () => { const trackEvent = vi.fn(); - const { getAddedHandler } = createReactionContext({ trackEvent }); - const addedHandler = getAddedHandler(); - expect(addedHandler).toBeTruthy(); - - await addedHandler!({ - event: makeReactionEvent(), - body: {}, - }); + await executeReactionCase({ trackEvent }); expect(trackEvent).toHaveBeenCalledTimes(1); }); diff --git a/src/slack/monitor/message-handler/prepare.test.ts b/src/slack/monitor/message-handler/prepare.test.ts index c41f821c02a..7a20f5568b8 100644 --- a/src/slack/monitor/message-handler/prepare.test.ts +++ b/src/slack/monitor/message-handler/prepare.test.ts @@ -189,6 +189,73 @@ describe("slack prepareSlackMessage inbound contract", () => { return prepareMessageWith(ctx, createThreadAccount(), createThreadReplyMessage(overrides)); } + function createDmScopeMainSlackCtx(): SlackMonitorContext { + const slackCtx = createInboundSlackCtx({ + cfg: { + channels: { slack: { enabled: true } }, + session: { dmScope: "main" }, + } as OpenClawConfig, + }); + // oxlint-disable-next-line typescript/no-explicit-any + slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; + // Simulate API returning correct type for DM channel + slackCtx.resolveChannelName = async () => ({ name: undefined, type: "im" as const }); + return slackCtx; + } + + function createMainScopedDmMessage(overrides: Partial): SlackMessageEvent { + return createSlackMessage({ + channel: "D0ACP6B1T8V", + user: "U1", + text: "hello from DM", + ts: "1.000", + ...overrides, + }); + } + + function expectMainScopedDmClassification( + prepared: Awaited>, + options?: { includeFromCheck?: boolean }, + ) { + expect(prepared).toBeTruthy(); + // oxlint-disable-next-line typescript/no-explicit-any + expectInboundContextContract(prepared!.ctxPayload as any); + expect(prepared!.isDirectMessage).toBe(true); + expect(prepared!.route.sessionKey).toBe("agent:main:main"); + expect(prepared!.ctxPayload.ChatType).toBe("direct"); + if (options?.includeFromCheck) { + expect(prepared!.ctxPayload.From).toContain("slack:U1"); + } + } + + function createReplyToAllSlackCtx(params?: { + groupPolicy?: "open"; + defaultRequireMention?: boolean; + asChannel?: boolean; + }): SlackMonitorContext { + const slackCtx = createInboundSlackCtx({ + cfg: { + channels: { + slack: { + enabled: true, + replyToMode: "all", + ...(params?.groupPolicy ? { groupPolicy: params.groupPolicy } : {}), + }, + }, + } as OpenClawConfig, + replyToMode: "all", + ...(params?.defaultRequireMention === undefined + ? {} + : { defaultRequireMention: params.defaultRequireMention }), + }); + // oxlint-disable-next-line typescript/no-explicit-any + slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; + if (params?.asChannel) { + slackCtx.resolveChannelName = async () => ({ name: "general", type: "channel" }); + } + return slackCtx; + } + it("produces a finalized MsgContext", async () => { const message: SlackMessageEvent = { channel: "D123", @@ -331,179 +398,34 @@ describe("slack prepareSlackMessage inbound contract", () => { }); it("classifies D-prefix DMs correctly even when channel_type is wrong", async () => { - const slackCtx = createSlackMonitorContext({ - cfg: { - channels: { slack: { enabled: true } }, - session: { dmScope: "main" }, - } as OpenClawConfig, - accountId: "default", - botToken: "token", - app: { client: {} } as App, - runtime: {} as RuntimeEnv, - botUserId: "B1", - teamId: "T1", - apiAppId: "A1", - historyLimit: 0, - sessionScope: "per-sender", - mainKey: "main", - dmEnabled: true, - dmPolicy: "open", - allowFrom: [], - allowNameMatching: false, - groupDmEnabled: true, - groupDmChannels: [], - defaultRequireMention: true, - groupPolicy: "open", - useAccessGroups: false, - reactionMode: "off", - reactionAllowlist: [], - replyToMode: "off", - threadHistoryScope: "thread", - threadInheritParent: false, - slashCommand: { - enabled: false, - name: "openclaw", - sessionPrefix: "slack:slash", - ephemeral: true, - }, - textLimit: 4000, - ackReactionScope: "group-mentions", - mediaMaxBytes: 1024, - removeAckAfterReply: false, - }); - // oxlint-disable-next-line typescript/no-explicit-any - slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; - // Simulate API returning correct type for DM channel - slackCtx.resolveChannelName = async () => ({ name: undefined, type: "im" as const }); + const prepared = await prepareMessageWith( + createDmScopeMainSlackCtx(), + createSlackAccount(), + createMainScopedDmMessage({ + // Bug scenario: D-prefix channel but Slack event says channel_type: "channel" + channel_type: "channel", + }), + ); - const account: ResolvedSlackAccount = { - accountId: "default", - enabled: true, - botTokenSource: "config", - appTokenSource: "config", - userTokenSource: "none", - config: {}, - }; - - // Bug scenario: D-prefix channel but Slack event says channel_type: "channel" - const message: SlackMessageEvent = { - channel: "D0ACP6B1T8V", - channel_type: "channel", - user: "U1", - text: "hello from DM", - ts: "1.000", - } as SlackMessageEvent; - - const prepared = await prepareSlackMessage({ - ctx: slackCtx, - account, - message, - opts: { source: "message" }, - }); - - expect(prepared).toBeTruthy(); - // oxlint-disable-next-line typescript/no-explicit-any - expectInboundContextContract(prepared!.ctxPayload as any); - // Should be classified as DM, not channel - expect(prepared!.isDirectMessage).toBe(true); - // DM with dmScope: "main" should route to the main session - expect(prepared!.route.sessionKey).toBe("agent:main:main"); - // ChatType should be "direct", not "channel" - expect(prepared!.ctxPayload.ChatType).toBe("direct"); - // From should use user ID (DM pattern), not channel ID - expect(prepared!.ctxPayload.From).toContain("slack:U1"); + expectMainScopedDmClassification(prepared, { includeFromCheck: true }); }); it("classifies D-prefix DMs when channel_type is missing", async () => { - const slackCtx = createSlackMonitorContext({ - cfg: { - channels: { slack: { enabled: true } }, - session: { dmScope: "main" }, - } as OpenClawConfig, - accountId: "default", - botToken: "token", - app: { client: {} } as App, - runtime: {} as RuntimeEnv, - botUserId: "B1", - teamId: "T1", - apiAppId: "A1", - historyLimit: 0, - sessionScope: "per-sender", - mainKey: "main", - dmEnabled: true, - dmPolicy: "open", - allowFrom: [], - allowNameMatching: false, - groupDmEnabled: true, - groupDmChannels: [], - defaultRequireMention: true, - groupPolicy: "open", - useAccessGroups: false, - reactionMode: "off", - reactionAllowlist: [], - replyToMode: "off", - threadHistoryScope: "thread", - threadInheritParent: false, - slashCommand: { - enabled: false, - name: "openclaw", - sessionPrefix: "slack:slash", - ephemeral: true, - }, - textLimit: 4000, - ackReactionScope: "group-mentions", - mediaMaxBytes: 1024, - removeAckAfterReply: false, - }); - // oxlint-disable-next-line typescript/no-explicit-any - slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; - // Simulate API returning correct type for DM channel - slackCtx.resolveChannelName = async () => ({ name: undefined, type: "im" as const }); - - const account: ResolvedSlackAccount = { - accountId: "default", - enabled: true, - botTokenSource: "config", - appTokenSource: "config", - userTokenSource: "none", - config: {}, - }; - - // channel_type missing — should infer from D-prefix - const message: SlackMessageEvent = { - channel: "D0ACP6B1T8V", - user: "U1", - text: "hello from DM", - ts: "1.000", - } as SlackMessageEvent; - - const prepared = await prepareSlackMessage({ - ctx: slackCtx, - account, + const message = createMainScopedDmMessage({}); + delete message.channel_type; + const prepared = await prepareMessageWith( + createDmScopeMainSlackCtx(), + createSlackAccount(), + // channel_type missing — should infer from D-prefix. message, - opts: { source: "message" }, - }); + ); - expect(prepared).toBeTruthy(); - // oxlint-disable-next-line typescript/no-explicit-any - expectInboundContextContract(prepared!.ctxPayload as any); - expect(prepared!.isDirectMessage).toBe(true); - expect(prepared!.route.sessionKey).toBe("agent:main:main"); - expect(prepared!.ctxPayload.ChatType).toBe("direct"); + expectMainScopedDmClassification(prepared); }); it("sets MessageThreadId for top-level messages when replyToMode=all", async () => { - const slackCtx = createInboundSlackCtx({ - cfg: { - channels: { slack: { enabled: true, replyToMode: "all" } }, - } as OpenClawConfig, - replyToMode: "all", - }); - // oxlint-disable-next-line typescript/no-explicit-any - slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; - const prepared = await prepareMessageWith( - slackCtx, + createReplyToAllSlackCtx(), createSlackAccount({ replyToMode: "all" }), createSlackMessage({}), ); @@ -513,17 +435,8 @@ describe("slack prepareSlackMessage inbound contract", () => { }); it("respects replyToModeByChatType.direct override for DMs", async () => { - const slackCtx = createInboundSlackCtx({ - cfg: { - channels: { slack: { enabled: true, replyToMode: "all" } }, - } as OpenClawConfig, - replyToMode: "all", - }); - // oxlint-disable-next-line typescript/no-explicit-any - slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; - const prepared = await prepareMessageWith( - slackCtx, + createReplyToAllSlackCtx(), createSlackAccount({ replyToMode: "all", replyToModeByChatType: { direct: "off" } }), createSlackMessage({}), // DM (channel_type: "im") ); @@ -534,19 +447,12 @@ describe("slack prepareSlackMessage inbound contract", () => { }); it("still threads channel messages when replyToModeByChatType.direct is off", async () => { - const slackCtx = createInboundSlackCtx({ - cfg: { - channels: { slack: { enabled: true, replyToMode: "all", groupPolicy: "open" } }, - } as OpenClawConfig, - replyToMode: "all", - defaultRequireMention: false, - }); - // oxlint-disable-next-line typescript/no-explicit-any - slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; - slackCtx.resolveChannelName = async () => ({ name: "general", type: "channel" }); - const prepared = await prepareMessageWith( - slackCtx, + createReplyToAllSlackCtx({ + groupPolicy: "open", + defaultRequireMention: false, + asChannel: true, + }), createSlackAccount({ replyToMode: "all", replyToModeByChatType: { direct: "off" } }), createSlackMessage({ channel: "C123", channel_type: "channel" }), ); @@ -557,17 +463,8 @@ describe("slack prepareSlackMessage inbound contract", () => { }); it("respects dm.replyToMode legacy override for DMs", async () => { - const slackCtx = createInboundSlackCtx({ - cfg: { - channels: { slack: { enabled: true, replyToMode: "all" } }, - } as OpenClawConfig, - replyToMode: "all", - }); - // oxlint-disable-next-line typescript/no-explicit-any - slackCtx.resolveUserName = async () => ({ name: "Alice" }) as any; - const prepared = await prepareMessageWith( - slackCtx, + createReplyToAllSlackCtx(), createSlackAccount({ replyToMode: "all", dm: { replyToMode: "off" } }), createSlackMessage({}), // DM ); diff --git a/src/telegram/bot-message-dispatch.sticker-media.test.ts b/src/telegram/bot-message-dispatch.sticker-media.test.ts index 5691bcfdde1..5e6cb118e88 100644 --- a/src/telegram/bot-message-dispatch.sticker-media.test.ts +++ b/src/telegram/bot-message-dispatch.sticker-media.test.ts @@ -1,9 +1,27 @@ import { describe, expect, it } from "vitest"; import { pruneStickerMediaFromContext } from "./bot-message-dispatch.js"; +type MediaCtx = { + MediaPath?: string; + MediaUrl?: string; + MediaType?: string; + MediaPaths?: string[]; + MediaUrls?: string[]; + MediaTypes?: string[]; +}; + +function expectSingleImageMedia(ctx: MediaCtx, mediaPath: string) { + expect(ctx.MediaPath).toBe(mediaPath); + expect(ctx.MediaUrl).toBe(mediaPath); + expect(ctx.MediaType).toBe("image/jpeg"); + expect(ctx.MediaPaths).toEqual([mediaPath]); + expect(ctx.MediaUrls).toEqual([mediaPath]); + expect(ctx.MediaTypes).toEqual(["image/jpeg"]); +} + describe("pruneStickerMediaFromContext", () => { it("preserves appended reply media while removing primary sticker media", () => { - const ctx = { + const ctx: MediaCtx = { MediaPath: "/tmp/sticker.webp", MediaUrl: "/tmp/sticker.webp", MediaType: "image/webp", @@ -14,16 +32,11 @@ describe("pruneStickerMediaFromContext", () => { pruneStickerMediaFromContext(ctx); - expect(ctx.MediaPath).toBe("/tmp/replied.jpg"); - expect(ctx.MediaUrl).toBe("/tmp/replied.jpg"); - expect(ctx.MediaType).toBe("image/jpeg"); - expect(ctx.MediaPaths).toEqual(["/tmp/replied.jpg"]); - expect(ctx.MediaUrls).toEqual(["/tmp/replied.jpg"]); - expect(ctx.MediaTypes).toEqual(["image/jpeg"]); + expectSingleImageMedia(ctx, "/tmp/replied.jpg"); }); it("clears media fields when sticker is the only media", () => { - const ctx = { + const ctx: MediaCtx = { MediaPath: "/tmp/sticker.webp", MediaUrl: "/tmp/sticker.webp", MediaType: "image/webp", @@ -43,7 +56,7 @@ describe("pruneStickerMediaFromContext", () => { }); it("does not prune when sticker media is already omitted from context", () => { - const ctx = { + const ctx: MediaCtx = { MediaPath: "/tmp/replied.jpg", MediaUrl: "/tmp/replied.jpg", MediaType: "image/jpeg", @@ -54,11 +67,6 @@ describe("pruneStickerMediaFromContext", () => { pruneStickerMediaFromContext(ctx, { stickerMediaIncluded: false }); - expect(ctx.MediaPath).toBe("/tmp/replied.jpg"); - expect(ctx.MediaUrl).toBe("/tmp/replied.jpg"); - expect(ctx.MediaType).toBe("image/jpeg"); - expect(ctx.MediaPaths).toEqual(["/tmp/replied.jpg"]); - expect(ctx.MediaUrls).toEqual(["/tmp/replied.jpg"]); - expect(ctx.MediaTypes).toEqual(["image/jpeg"]); + expectSingleImageMedia(ctx, "/tmp/replied.jpg"); }); }); diff --git a/src/telegram/webhook.test.ts b/src/telegram/webhook.test.ts index 80d25428011..4430a571408 100644 --- a/src/telegram/webhook.test.ts +++ b/src/telegram/webhook.test.ts @@ -20,6 +20,9 @@ const createTelegramBotSpy = vi.hoisted(() => ); const WEBHOOK_POST_TIMEOUT_MS = process.platform === "win32" ? 20_000 : 8_000; +const TELEGRAM_TOKEN = "tok"; +const TELEGRAM_SECRET = "secret"; +const TELEGRAM_WEBHOOK_PATH = "/hook"; vi.mock("grammy", async (importOriginal) => { const actual = await importOriginal(); @@ -202,96 +205,175 @@ function sha256(text: string): string { return createHash("sha256").update(text).digest("hex"); } +type StartWebhookOptions = Omit< + Parameters[0], + "token" | "port" | "abortSignal" +>; + +type StartedWebhook = Awaited>; + +function getServerPort(server: StartedWebhook["server"]): number { + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("no addr"); + } + return address.port; +} + +function webhookUrl(port: number, webhookPath: string): string { + return `http://127.0.0.1:${port}${webhookPath}`; +} + +async function withStartedWebhook( + options: StartWebhookOptions, + run: (ctx: { server: StartedWebhook["server"]; port: number }) => Promise, +): Promise { + const abort = new AbortController(); + const started = await startTelegramWebhook({ + token: TELEGRAM_TOKEN, + port: 0, + abortSignal: abort.signal, + ...options, + }); + try { + return await run({ server: started.server, port: getServerPort(started.server) }); + } finally { + abort.abort(); + } +} + +function expectSingleNearLimitUpdate(params: { + seenUpdates: Array<{ update_id: number; message: { text: string } }>; + expected: { update_id: number; message: { text: string } }; +}) { + expect(params.seenUpdates).toHaveLength(1); + expect(params.seenUpdates[0]?.update_id).toBe(params.expected.update_id); + expect(params.seenUpdates[0]?.message.text.length).toBe(params.expected.message.text.length); + expect(sha256(params.seenUpdates[0]?.message.text ?? "")).toBe( + sha256(params.expected.message.text), + ); +} + +async function runNearLimitPayloadTest(mode: "single" | "random-chunked"): Promise { + const seenUpdates: Array<{ update_id: number; message: { text: string } }> = []; + webhookCallbackSpy.mockImplementationOnce( + () => + vi.fn( + ( + update: unknown, + reply: (json: string) => Promise, + _secretHeader: string | undefined, + _unauthorized: () => Promise, + ) => { + seenUpdates.push(update as { update_id: number; message: { text: string } }); + void reply("ok"); + }, + ) as unknown as typeof handlerSpy, + ); + + const { payload, sizeBytes } = createNearLimitTelegramPayload(); + expect(sizeBytes).toBeLessThan(1_024 * 1_024); + expect(sizeBytes).toBeGreaterThan(256 * 1_024); + const expected = JSON.parse(payload) as { update_id: number; message: { text: string } }; + + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, + path: TELEGRAM_WEBHOOK_PATH, + }, + async ({ port }) => { + const response = await postWebhookPayloadWithChunkPlan({ + port, + path: TELEGRAM_WEBHOOK_PATH, + payload, + secret: TELEGRAM_SECRET, + mode, + timeoutMs: WEBHOOK_POST_TIMEOUT_MS, + }); + + expect(response.statusCode).toBe(200); + expectSingleNearLimitUpdate({ seenUpdates, expected }); + }, + ); +} + describe("startTelegramWebhook", () => { it("starts server, registers webhook, and serves health", async () => { initSpy.mockClear(); createTelegramBotSpy.mockClear(); webhookCallbackSpy.mockClear(); const runtimeLog = vi.fn(); - const abort = new AbortController(); const cfg = { bindings: [] }; - const { server } = await startTelegramWebhook({ - token: "tok", - secret: "secret", - accountId: "opie", - config: cfg, - port: 0, // random free port - abortSignal: abort.signal, - runtime: { log: runtimeLog, error: vi.fn(), exit: vi.fn() }, - }); - expect(createTelegramBotSpy).toHaveBeenCalledWith( - expect.objectContaining({ - accountId: "opie", - config: expect.objectContaining({ bindings: [] }), - }), - ); - const address = server.address(); - if (!address || typeof address === "string") { - throw new Error("no address"); - } - const url = `http://127.0.0.1:${address.port}`; - - const health = await fetch(`${url}/healthz`); - expect(health.status).toBe(200); - expect(initSpy).toHaveBeenCalledTimes(1); - expect(setWebhookSpy).toHaveBeenCalled(); - expect(webhookCallbackSpy).toHaveBeenCalledWith( - expect.objectContaining({ - api: expect.objectContaining({ - setWebhook: expect.any(Function), - }), - }), - "callback", + await withStartedWebhook( { - secretToken: "secret", - onTimeout: "return", - timeoutMilliseconds: 10_000, + secret: TELEGRAM_SECRET, + accountId: "opie", + config: cfg, + runtime: { log: runtimeLog, error: vi.fn(), exit: vi.fn() }, + }, + async ({ port }) => { + expect(createTelegramBotSpy).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: "opie", + config: expect.objectContaining({ bindings: [] }), + }), + ); + const health = await fetch(`http://127.0.0.1:${port}/healthz`); + expect(health.status).toBe(200); + expect(initSpy).toHaveBeenCalledTimes(1); + expect(setWebhookSpy).toHaveBeenCalled(); + expect(webhookCallbackSpy).toHaveBeenCalledWith( + expect.objectContaining({ + api: expect.objectContaining({ + setWebhook: expect.any(Function), + }), + }), + "callback", + { + secretToken: TELEGRAM_SECRET, + onTimeout: "return", + timeoutMilliseconds: 10_000, + }, + ); + expect(runtimeLog).toHaveBeenCalledWith( + expect.stringContaining("webhook local listener on http://127.0.0.1:"), + ); + expect(runtimeLog).toHaveBeenCalledWith(expect.stringContaining("/telegram-webhook")); + expect(runtimeLog).toHaveBeenCalledWith( + expect.stringContaining("webhook advertised to telegram on http://"), + ); }, ); - expect(runtimeLog).toHaveBeenCalledWith( - expect.stringContaining("webhook local listener on http://127.0.0.1:"), - ); - expect(runtimeLog).toHaveBeenCalledWith(expect.stringContaining("/telegram-webhook")); - expect(runtimeLog).toHaveBeenCalledWith( - expect.stringContaining("webhook advertised to telegram on http://"), - ); - - abort.abort(); }); it("invokes webhook handler on matching path", async () => { handlerSpy.mockClear(); createTelegramBotSpy.mockClear(); - const abort = new AbortController(); const cfg = { bindings: [] }; - const { server } = await startTelegramWebhook({ - token: "tok", - secret: "secret", - accountId: "opie", - config: cfg, - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); - expect(createTelegramBotSpy).toHaveBeenCalledWith( - expect.objectContaining({ + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, accountId: "opie", - config: expect.objectContaining({ bindings: [] }), - }), + config: cfg, + path: TELEGRAM_WEBHOOK_PATH, + }, + async ({ port }) => { + expect(createTelegramBotSpy).toHaveBeenCalledWith( + expect.objectContaining({ + accountId: "opie", + config: expect.objectContaining({ bindings: [] }), + }), + ); + const payload = JSON.stringify({ update_id: 1, message: { text: "hello" } }); + const response = await postWebhookJson({ + url: webhookUrl(port, TELEGRAM_WEBHOOK_PATH), + payload, + secret: TELEGRAM_SECRET, + }); + expect(response.status).toBe(200); + expect(handlerSpy).toHaveBeenCalled(); + }, ); - const addr = server.address(); - if (!addr || typeof addr === "string") { - throw new Error("no addr"); - } - const payload = JSON.stringify({ update_id: 1, message: { text: "hello" } }); - const response = await postWebhookJson({ - url: `http://127.0.0.1:${addr.port}/hook`, - payload, - secret: "secret", - }); - expect(response.status).toBe(200); - expect(handlerSpy).toHaveBeenCalled(); - abort.abort(); }); it("rejects startup when webhook secret is missing", async () => { @@ -305,34 +387,26 @@ describe("startTelegramWebhook", () => { it("registers webhook using the bound listening port when port is 0", async () => { setWebhookSpy.mockClear(); const runtimeLog = vi.fn(); - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret: "secret", - port: 0, - abortSignal: abort.signal, - path: "/hook", - runtime: { log: runtimeLog, error: vi.fn(), exit: vi.fn() }, - }); - try { - const addr = server.address(); - if (!addr || typeof addr === "string") { - throw new Error("no addr"); - } - expect(addr.port).toBeGreaterThan(0); - expect(setWebhookSpy).toHaveBeenCalledTimes(1); - expect(setWebhookSpy).toHaveBeenCalledWith( - `http://127.0.0.1:${addr.port}/hook`, - expect.objectContaining({ - secret_token: "secret", - }), - ); - expect(runtimeLog).toHaveBeenCalledWith( - `webhook local listener on http://127.0.0.1:${addr.port}/hook`, - ); - } finally { - abort.abort(); - } + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, + path: TELEGRAM_WEBHOOK_PATH, + runtime: { log: runtimeLog, error: vi.fn(), exit: vi.fn() }, + }, + async ({ port }) => { + expect(port).toBeGreaterThan(0); + expect(setWebhookSpy).toHaveBeenCalledTimes(1); + expect(setWebhookSpy).toHaveBeenCalledWith( + webhookUrl(port, TELEGRAM_WEBHOOK_PATH), + expect.objectContaining({ + secret_token: TELEGRAM_SECRET, + }), + ); + expect(runtimeLog).toHaveBeenCalledWith( + `webhook local listener on ${webhookUrl(port, TELEGRAM_WEBHOOK_PATH)}`, + ); + }, + ); }); it("keeps webhook payload readable when callback delays body read", async () => { @@ -342,32 +416,23 @@ describe("startTelegramWebhook", () => { await reply(JSON.stringify(update)); }); - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret: "secret", - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); - try { - const addr = server.address(); - if (!addr || typeof addr === "string") { - throw new Error("no addr"); - } - - const payload = JSON.stringify({ update_id: 1, message: { text: "hello" } }); - const res = await postWebhookJson({ - url: `http://127.0.0.1:${addr.port}/hook`, - payload, - secret: "secret", - }); - expect(res.status).toBe(200); - const responseBody = await res.text(); - expect(JSON.parse(responseBody)).toEqual(JSON.parse(payload)); - } finally { - abort.abort(); - } + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, + path: TELEGRAM_WEBHOOK_PATH, + }, + async ({ port }) => { + const payload = JSON.stringify({ update_id: 1, message: { text: "hello" } }); + const res = await postWebhookJson({ + url: webhookUrl(port, TELEGRAM_WEBHOOK_PATH), + payload, + secret: TELEGRAM_SECRET, + }); + expect(res.status).toBe(200); + const responseBody = await res.text(); + expect(JSON.parse(responseBody)).toEqual(JSON.parse(payload)); + }, + ); }); it("keeps webhook payload readable across multiple delayed reads", async () => { @@ -380,38 +445,29 @@ describe("startTelegramWebhook", () => { }; handlerSpy.mockImplementationOnce(delayedHandler).mockImplementationOnce(delayedHandler); - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret: "secret", - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); - try { - const addr = server.address(); - if (!addr || typeof addr === "string") { - throw new Error("no addr"); - } + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, + path: TELEGRAM_WEBHOOK_PATH, + }, + async ({ port }) => { + const payloads = [ + JSON.stringify({ update_id: 1, message: { text: "first" } }), + JSON.stringify({ update_id: 2, message: { text: "second" } }), + ]; - const payloads = [ - JSON.stringify({ update_id: 1, message: { text: "first" } }), - JSON.stringify({ update_id: 2, message: { text: "second" } }), - ]; + for (const payload of payloads) { + const res = await postWebhookJson({ + url: webhookUrl(port, TELEGRAM_WEBHOOK_PATH), + payload, + secret: TELEGRAM_SECRET, + }); + expect(res.status).toBe(200); + } - for (const payload of payloads) { - const res = await postWebhookJson({ - url: `http://127.0.0.1:${addr.port}/hook`, - payload, - secret: "secret", - }); - expect(res.status).toBe(200); - } - - expect(seenPayloads.map((x) => JSON.parse(x))).toEqual(payloads.map((x) => JSON.parse(x))); - } finally { - abort.abort(); - } + expect(seenPayloads.map((x) => JSON.parse(x))).toEqual(payloads.map((x) => JSON.parse(x))); + }, + ); }); it("processes a second request after first-request delayed-init data loss", async () => { @@ -434,237 +490,110 @@ describe("startTelegramWebhook", () => { ) as unknown as typeof handlerSpy, ); - const secret = "secret"; - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret, - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, + path: TELEGRAM_WEBHOOK_PATH, + }, + async ({ port }) => { + const firstPayload = JSON.stringify({ update_id: 100, message: { text: "first" } }); + const secondPayload = JSON.stringify({ update_id: 101, message: { text: "second" } }); + const firstResponse = await postWebhookPayloadWithChunkPlan({ + port, + path: TELEGRAM_WEBHOOK_PATH, + payload: firstPayload, + secret: TELEGRAM_SECRET, + mode: "single", + timeoutMs: WEBHOOK_POST_TIMEOUT_MS, + }); + const secondResponse = await postWebhookPayloadWithChunkPlan({ + port, + path: TELEGRAM_WEBHOOK_PATH, + payload: secondPayload, + secret: TELEGRAM_SECRET, + mode: "single", + timeoutMs: WEBHOOK_POST_TIMEOUT_MS, + }); - try { - const address = server.address(); - if (!address || typeof address === "string") { - throw new Error("no addr"); - } - - const firstPayload = JSON.stringify({ update_id: 100, message: { text: "first" } }); - const secondPayload = JSON.stringify({ update_id: 101, message: { text: "second" } }); - const firstResponse = await postWebhookPayloadWithChunkPlan({ - port: address.port, - path: "/hook", - payload: firstPayload, - secret, - mode: "single", - timeoutMs: WEBHOOK_POST_TIMEOUT_MS, - }); - const secondResponse = await postWebhookPayloadWithChunkPlan({ - port: address.port, - path: "/hook", - payload: secondPayload, - secret, - mode: "single", - timeoutMs: WEBHOOK_POST_TIMEOUT_MS, - }); - - expect(firstResponse.statusCode).toBe(200); - expect(secondResponse.statusCode).toBe(200); - expect(seenUpdates).toEqual([JSON.parse(firstPayload), JSON.parse(secondPayload)]); - } finally { - abort.abort(); - } + expect(firstResponse.statusCode).toBe(200); + expect(secondResponse.statusCode).toBe(200); + expect(seenUpdates).toEqual([JSON.parse(firstPayload), JSON.parse(secondPayload)]); + }, + ); }); it("handles near-limit payload with random chunk writes and event-loop yields", async () => { - const seenUpdates: Array<{ update_id: number; message: { text: string } }> = []; - webhookCallbackSpy.mockImplementationOnce( - () => - vi.fn( - ( - update: unknown, - reply: (json: string) => Promise, - _secretHeader: string | undefined, - _unauthorized: () => Promise, - ) => { - seenUpdates.push(update as { update_id: number; message: { text: string } }); - void reply("ok"); - }, - ) as unknown as typeof handlerSpy, - ); - - const { payload, sizeBytes } = createNearLimitTelegramPayload(); - expect(sizeBytes).toBeLessThan(1_024 * 1_024); - expect(sizeBytes).toBeGreaterThan(256 * 1_024); - const expected = JSON.parse(payload) as { update_id: number; message: { text: string } }; - - const secret = "secret"; - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret, - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); - - try { - const address = server.address(); - if (!address || typeof address === "string") { - throw new Error("no addr"); - } - - const response = await postWebhookPayloadWithChunkPlan({ - port: address.port, - path: "/hook", - payload, - secret, - mode: "random-chunked", - timeoutMs: WEBHOOK_POST_TIMEOUT_MS, - }); - - expect(response.statusCode).toBe(200); - expect(seenUpdates).toHaveLength(1); - expect(seenUpdates[0]?.update_id).toBe(expected.update_id); - expect(seenUpdates[0]?.message.text.length).toBe(expected.message.text.length); - expect(sha256(seenUpdates[0]?.message.text ?? "")).toBe(sha256(expected.message.text)); - } finally { - abort.abort(); - } + await runNearLimitPayloadTest("random-chunked"); }); it("handles near-limit payload written in a single request write", async () => { - const seenUpdates: Array<{ update_id: number; message: { text: string } }> = []; - webhookCallbackSpy.mockImplementationOnce( - () => - vi.fn( - ( - update: unknown, - reply: (json: string) => Promise, - _secretHeader: string | undefined, - _unauthorized: () => Promise, - ) => { - seenUpdates.push(update as { update_id: number; message: { text: string } }); - void reply("ok"); - }, - ) as unknown as typeof handlerSpy, - ); - - const { payload, sizeBytes } = createNearLimitTelegramPayload(); - expect(sizeBytes).toBeLessThan(1_024 * 1_024); - expect(sizeBytes).toBeGreaterThan(256 * 1_024); - const expected = JSON.parse(payload) as { update_id: number; message: { text: string } }; - - const secret = "secret"; - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret, - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); - - try { - const address = server.address(); - if (!address || typeof address === "string") { - throw new Error("no addr"); - } - - const response = await postWebhookPayloadWithChunkPlan({ - port: address.port, - path: "/hook", - payload, - secret, - mode: "single", - timeoutMs: WEBHOOK_POST_TIMEOUT_MS, - }); - - expect(response.statusCode).toBe(200); - expect(seenUpdates).toHaveLength(1); - expect(seenUpdates[0]?.update_id).toBe(expected.update_id); - expect(seenUpdates[0]?.message.text.length).toBe(expected.message.text.length); - expect(sha256(seenUpdates[0]?.message.text ?? "")).toBe(sha256(expected.message.text)); - } finally { - abort.abort(); - } + await runNearLimitPayloadTest("single"); }); it("rejects payloads larger than 1MB before invoking webhook handler", async () => { handlerSpy.mockClear(); - const abort = new AbortController(); - const { server } = await startTelegramWebhook({ - token: "tok", - secret: "secret", - port: 0, - abortSignal: abort.signal, - path: "/hook", - }); - - try { - const address = server.address(); - if (!address || typeof address === "string") { - throw new Error("no addr"); - } - - const responseOrError = await new Promise< - | { kind: "response"; statusCode: number; body: string } - | { kind: "error"; code: string | undefined } - >((resolve) => { - const req = request( - { - hostname: "127.0.0.1", - port: address.port, - path: "/hook", - method: "POST", - headers: { - "content-type": "application/json", - "content-length": String(1_024 * 1_024 + 2_048), - "x-telegram-bot-api-secret-token": "secret", + await withStartedWebhook( + { + secret: TELEGRAM_SECRET, + path: TELEGRAM_WEBHOOK_PATH, + }, + async ({ port }) => { + const responseOrError = await new Promise< + | { kind: "response"; statusCode: number; body: string } + | { kind: "error"; code: string | undefined } + >((resolve) => { + const req = request( + { + hostname: "127.0.0.1", + port, + path: TELEGRAM_WEBHOOK_PATH, + method: "POST", + headers: { + "content-type": "application/json", + "content-length": String(1_024 * 1_024 + 2_048), + "x-telegram-bot-api-secret-token": TELEGRAM_SECRET, + }, }, - }, - (res) => { - const chunks: Buffer[] = []; - res.on("data", (chunk: Buffer | string) => { - chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); - }); - res.on("end", () => { - resolve({ - kind: "response", - statusCode: res.statusCode ?? 0, - body: Buffer.concat(chunks).toString("utf-8"), + (res) => { + const chunks: Buffer[] = []; + res.on("data", (chunk: Buffer | string) => { + chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); }); - }); - }, - ); - req.on("error", (error: NodeJS.ErrnoException) => { - resolve({ kind: "error", code: error.code }); + res.on("end", () => { + resolve({ + kind: "response", + statusCode: res.statusCode ?? 0, + body: Buffer.concat(chunks).toString("utf-8"), + }); + }); + }, + ); + req.on("error", (error: NodeJS.ErrnoException) => { + resolve({ kind: "error", code: error.code }); + }); + req.end("{}"); }); - req.end("{}"); - }); - if (responseOrError.kind === "response") { - expect(responseOrError.statusCode).toBe(413); - expect(responseOrError.body).toBe("Payload too large"); - } else { - expect(responseOrError.code).toBeOneOf(["ECONNRESET", "EPIPE"]); - } - expect(handlerSpy).not.toHaveBeenCalled(); - } finally { - abort.abort(); - } + if (responseOrError.kind === "response") { + expect(responseOrError.statusCode).toBe(413); + expect(responseOrError.body).toBe("Payload too large"); + } else { + expect(responseOrError.code).toBeOneOf(["ECONNRESET", "EPIPE"]); + } + expect(handlerSpy).not.toHaveBeenCalled(); + }, + ); }); it("de-registers webhook when shutting down", async () => { deleteWebhookSpy.mockClear(); const abort = new AbortController(); await startTelegramWebhook({ - token: "tok", - secret: "secret", + token: TELEGRAM_TOKEN, + secret: TELEGRAM_SECRET, port: 0, abortSignal: abort.signal, - path: "/hook", + path: TELEGRAM_WEBHOOK_PATH, }); abort.abort(); diff --git a/src/tui/tui-local-shell.test.ts b/src/tui/tui-local-shell.test.ts index 0c8f324c3b3..62272cf0601 100644 --- a/src/tui/tui-local-shell.test.ts +++ b/src/tui/tui-local-shell.test.ts @@ -12,62 +12,63 @@ const createSelector = () => { return selector; }; +function createShellHarness(params?: { + spawnCommand?: typeof import("node:child_process").spawn; + env?: Record; +}) { + const messages: string[] = []; + const chatLog = { + addSystem: (line: string) => { + messages.push(line); + }, + }; + const tui = { requestRender: vi.fn() }; + const openOverlay = vi.fn(); + const closeOverlay = vi.fn(); + let lastSelector: ReturnType | null = null; + const createSelectorSpy = vi.fn(() => { + lastSelector = createSelector(); + return lastSelector; + }); + const spawnCommand = params?.spawnCommand ?? vi.fn(); + const { runLocalShellLine } = createLocalShellRunner({ + chatLog, + tui, + openOverlay, + closeOverlay, + createSelector: createSelectorSpy, + spawnCommand, + ...(params?.env ? { env: params.env } : {}), + }); + return { + messages, + openOverlay, + createSelectorSpy, + spawnCommand, + runLocalShellLine, + getLastSelector: () => lastSelector, + }; +} + describe("createLocalShellRunner", () => { it("logs denial on subsequent ! attempts without re-prompting", async () => { - const messages: string[] = []; - const chatLog = { - addSystem: (line: string) => { - messages.push(line); - }, - }; - const tui = { requestRender: vi.fn() }; - const openOverlay = vi.fn(); - const closeOverlay = vi.fn(); - let lastSelector: ReturnType | null = null; - const createSelectorSpy = vi.fn(() => { - lastSelector = createSelector(); - return lastSelector; - }); - const spawnCommand = vi.fn(); + const harness = createShellHarness(); - const { runLocalShellLine } = createLocalShellRunner({ - chatLog, - tui, - openOverlay, - closeOverlay, - createSelector: createSelectorSpy, - spawnCommand, - }); - - const firstRun = runLocalShellLine("!ls"); - expect(openOverlay).toHaveBeenCalledTimes(1); - const selector = lastSelector as ReturnType | null; + const firstRun = harness.runLocalShellLine("!ls"); + expect(harness.openOverlay).toHaveBeenCalledTimes(1); + const selector = harness.getLastSelector(); selector?.onSelect?.({ value: "no", label: "No" }); await firstRun; - await runLocalShellLine("!pwd"); + await harness.runLocalShellLine("!pwd"); - expect(messages).toContain("local shell: not enabled"); - expect(messages).toContain("local shell: not enabled for this session"); - expect(createSelectorSpy).toHaveBeenCalledTimes(1); - expect(spawnCommand).not.toHaveBeenCalled(); + expect(harness.messages).toContain("local shell: not enabled"); + expect(harness.messages).toContain("local shell: not enabled for this session"); + expect(harness.createSelectorSpy).toHaveBeenCalledTimes(1); + expect(harness.spawnCommand).not.toHaveBeenCalled(); }); it("sets OPENCLAW_SHELL when running local shell commands", async () => { - const messages: string[] = []; - const chatLog = { - addSystem: (line: string) => { - messages.push(line); - }, - }; - const tui = { requestRender: vi.fn() }; - const openOverlay = vi.fn(); - const closeOverlay = vi.fn(); - let lastSelector: ReturnType | null = null; - const createSelectorSpy = vi.fn(() => { - lastSelector = createSelector(); - return lastSelector; - }); const spawnCommand = vi.fn((_command: string, _options: unknown) => { const stdout = new EventEmitter(); const stderr = new EventEmitter(); @@ -82,27 +83,22 @@ describe("createLocalShellRunner", () => { }; }); - const { runLocalShellLine } = createLocalShellRunner({ - chatLog, - tui, - openOverlay, - closeOverlay, - createSelector: createSelectorSpy, + const harness = createShellHarness({ spawnCommand: spawnCommand as unknown as typeof import("node:child_process").spawn, env: { PATH: "/tmp/bin", USER: "dev" }, }); - const firstRun = runLocalShellLine("!echo hi"); - expect(openOverlay).toHaveBeenCalledTimes(1); - const selector = lastSelector as ReturnType | null; + const firstRun = harness.runLocalShellLine("!echo hi"); + expect(harness.openOverlay).toHaveBeenCalledTimes(1); + const selector = harness.getLastSelector(); selector?.onSelect?.({ value: "yes", label: "Yes" }); await firstRun; - expect(createSelectorSpy).toHaveBeenCalledTimes(1); + expect(harness.createSelectorSpy).toHaveBeenCalledTimes(1); expect(spawnCommand).toHaveBeenCalledTimes(1); const spawnOptions = spawnCommand.mock.calls[0]?.[1] as { env?: Record }; expect(spawnOptions.env?.OPENCLAW_SHELL).toBe("tui-local"); expect(spawnOptions.env?.PATH).toBe("/tmp/bin"); - expect(messages).toContain("local shell: enabled for this session"); + expect(harness.messages).toContain("local shell: enabled for this session"); }); });