From 3efd224ec63d2688d1b494654e0920abd7e987d9 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 14:35:26 +0000 Subject: [PATCH] refactor(commands): dedupe session target resolution and fs tool test setup --- .../pi-tools.workspace-only-false.test.ts | 243 +++++++----------- src/commands/session-store-targets.ts | 15 ++ src/commands/sessions-cleanup.test.ts | 13 + src/commands/sessions-cleanup.ts | 19 +- src/commands/sessions.ts | 16 +- 5 files changed, 137 insertions(+), 169 deletions(-) diff --git a/src/agents/pi-tools.workspace-only-false.test.ts b/src/agents/pi-tools.workspace-only-false.test.ts index da08f2a80..713315de8 100644 --- a/src/agents/pi-tools.workspace-only-false.test.ts +++ b/src/agents/pi-tools.workspace-only-false.test.ts @@ -9,6 +9,42 @@ describe("FS tools with workspaceOnly=false", () => { let workspaceDir: string; let outsideFile: string; + const hasToolError = (result: { content: Array<{ type: string; text?: string }> }) => + result.content.some((content) => { + if (content.type !== "text") { + return false; + } + return content.text?.toLowerCase().includes("error") ?? false; + }); + + const toolsFor = (workspaceOnly: boolean | undefined) => + createOpenClawCodingTools({ + workspaceDir, + config: + workspaceOnly === undefined + ? {} + : { + tools: { + fs: { + workspaceOnly, + }, + }, + }, + }); + + const runFsTool = async ( + toolName: "write" | "edit" | "read", + callId: string, + input: Record, + workspaceOnly: boolean | undefined, + ) => { + const tool = toolsFor(workspaceOnly).find((candidate) => candidate.name === toolName); + expect(tool).toBeDefined(); + const result = await tool!.execute(callId, input); + expect(hasToolError(result)).toBe(false); + return result; + }; + beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-test-")); workspaceDir = path.join(tmpDir, "workspace"); @@ -21,30 +57,15 @@ describe("FS tools with workspaceOnly=false", () => { }); it("should allow write outside workspace when workspaceOnly=false", async () => { - const tools = createOpenClawCodingTools({ - workspaceDir, - config: { - tools: { - fs: { - workspaceOnly: false, - }, - }, + await runFsTool( + "write", + "test-call-1", + { + path: outsideFile, + content: "test content", }, - }); - - const writeTool = tools.find((t) => t.name === "write"); - expect(writeTool).toBeDefined(); - - const result = await writeTool!.execute("test-call-1", { - path: outsideFile, - content: "test content", - }); - - // Check if the operation succeeded (no error in content) - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + false, ); - expect(hasError).toBe(false); const content = await fs.readFile(outsideFile, "utf-8"); expect(content).toBe("test content"); }); @@ -53,29 +74,15 @@ describe("FS tools with workspaceOnly=false", () => { const relativeOutsidePath = path.join("..", "outside-relative-write.txt"); const outsideRelativeFile = path.join(tmpDir, "outside-relative-write.txt"); - const tools = createOpenClawCodingTools({ - workspaceDir, - config: { - tools: { - fs: { - workspaceOnly: false, - }, - }, + await runFsTool( + "write", + "test-call-1b", + { + path: relativeOutsidePath, + content: "relative test content", }, - }); - - const writeTool = tools.find((t) => t.name === "write"); - expect(writeTool).toBeDefined(); - - const result = await writeTool!.execute("test-call-1b", { - path: relativeOutsidePath, - content: "relative test content", - }); - - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + false, ); - expect(hasError).toBe(false); const content = await fs.readFile(outsideRelativeFile, "utf-8"); expect(content).toBe("relative test content"); }); @@ -83,31 +90,16 @@ describe("FS tools with workspaceOnly=false", () => { it("should allow edit outside workspace when workspaceOnly=false", async () => { await fs.writeFile(outsideFile, "old content"); - const tools = createOpenClawCodingTools({ - workspaceDir, - config: { - tools: { - fs: { - workspaceOnly: false, - }, - }, + await runFsTool( + "edit", + "test-call-2", + { + path: outsideFile, + oldText: "old content", + newText: "new content", }, - }); - - const editTool = tools.find((t) => t.name === "edit"); - expect(editTool).toBeDefined(); - - const result = await editTool!.execute("test-call-2", { - path: outsideFile, - oldText: "old content", - newText: "new content", - }); - - // Check if the operation succeeded (no error in content) - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + false, ); - expect(hasError).toBe(false); const content = await fs.readFile(outsideFile, "utf-8"); expect(content).toBe("new content"); }); @@ -117,30 +109,16 @@ describe("FS tools with workspaceOnly=false", () => { const outsideRelativeFile = path.join(tmpDir, "outside-relative-edit.txt"); await fs.writeFile(outsideRelativeFile, "old relative content"); - const tools = createOpenClawCodingTools({ - workspaceDir, - config: { - tools: { - fs: { - workspaceOnly: false, - }, - }, + await runFsTool( + "edit", + "test-call-2b", + { + path: relativeOutsidePath, + oldText: "old relative content", + newText: "new relative content", }, - }); - - const editTool = tools.find((t) => t.name === "edit"); - expect(editTool).toBeDefined(); - - const result = await editTool!.execute("test-call-2b", { - path: relativeOutsidePath, - oldText: "old relative content", - newText: "new relative content", - }); - - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + false, ); - expect(hasError).toBe(false); const content = await fs.readFile(outsideRelativeFile, "utf-8"); expect(content).toBe("new relative content"); }); @@ -148,50 +126,27 @@ describe("FS tools with workspaceOnly=false", () => { it("should allow read outside workspace when workspaceOnly=false", async () => { await fs.writeFile(outsideFile, "test read content"); - const tools = createOpenClawCodingTools({ - workspaceDir, - config: { - tools: { - fs: { - workspaceOnly: false, - }, - }, + await runFsTool( + "read", + "test-call-3", + { + path: outsideFile, }, - }); - - const readTool = tools.find((t) => t.name === "read"); - expect(readTool).toBeDefined(); - - const result = await readTool!.execute("test-call-3", { - path: outsideFile, - }); - - // Check if the operation succeeded (no error in content) - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + false, ); - expect(hasError).toBe(false); }); it("should allow write outside workspace when workspaceOnly is unset", async () => { const outsideUnsetFile = path.join(tmpDir, "outside-unset-write.txt"); - const tools = createOpenClawCodingTools({ - workspaceDir, - config: {}, - }); - - const writeTool = tools.find((t) => t.name === "write"); - expect(writeTool).toBeDefined(); - - const result = await writeTool!.execute("test-call-3a", { - path: outsideUnsetFile, - content: "unset write content", - }); - - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + await runFsTool( + "write", + "test-call-3a", + { + path: outsideUnsetFile, + content: "unset write content", + }, + undefined, ); - expect(hasError).toBe(false); const content = await fs.readFile(outsideUnsetFile, "utf-8"); expect(content).toBe("unset write content"); }); @@ -199,40 +154,22 @@ describe("FS tools with workspaceOnly=false", () => { it("should allow edit outside workspace when workspaceOnly is unset", async () => { const outsideUnsetFile = path.join(tmpDir, "outside-unset-edit.txt"); await fs.writeFile(outsideUnsetFile, "before"); - const tools = createOpenClawCodingTools({ - workspaceDir, - config: {}, - }); - - const editTool = tools.find((t) => t.name === "edit"); - expect(editTool).toBeDefined(); - - const result = await editTool!.execute("test-call-3b", { - path: outsideUnsetFile, - oldText: "before", - newText: "after", - }); - - const hasError = result.content.some( - (c) => c.type === "text" && c.text.toLowerCase().includes("error"), + await runFsTool( + "edit", + "test-call-3b", + { + path: outsideUnsetFile, + oldText: "before", + newText: "after", + }, + undefined, ); - expect(hasError).toBe(false); const content = await fs.readFile(outsideUnsetFile, "utf-8"); expect(content).toBe("after"); }); it("should block write outside workspace when workspaceOnly=true", async () => { - const tools = createOpenClawCodingTools({ - workspaceDir, - config: { - tools: { - fs: { - workspaceOnly: true, - }, - }, - }, - }); - + const tools = toolsFor(true); const writeTool = tools.find((t) => t.name === "write"); expect(writeTool).toBeDefined(); diff --git a/src/commands/session-store-targets.ts b/src/commands/session-store-targets.ts index 5c70af85b..c9e91006e 100644 --- a/src/commands/session-store-targets.ts +++ b/src/commands/session-store-targets.ts @@ -2,6 +2,7 @@ import { listAgentIds, resolveDefaultAgentId } from "../agents/agent-scope.js"; import { resolveStorePath } from "../config/sessions.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { normalizeAgentId } from "../routing/session-key.js"; +import type { RuntimeEnv } from "../runtime.js"; export type SessionStoreSelectionOptions = { store?: string; @@ -78,3 +79,17 @@ export function resolveSessionStoreTargets( }, ]; } + +export function resolveSessionStoreTargetsOrExit(params: { + cfg: OpenClawConfig; + opts: SessionStoreSelectionOptions; + runtime: RuntimeEnv; +}): SessionStoreTarget[] | null { + try { + return resolveSessionStoreTargets(params.cfg, params.opts); + } catch (error) { + params.runtime.error(error instanceof Error ? error.message : String(error)); + params.runtime.exit(1); + return null; + } +} diff --git a/src/commands/sessions-cleanup.test.ts b/src/commands/sessions-cleanup.test.ts index 6dc9556ca..5f593d34b 100644 --- a/src/commands/sessions-cleanup.test.ts +++ b/src/commands/sessions-cleanup.test.ts @@ -5,6 +5,7 @@ import type { RuntimeEnv } from "../runtime.js"; const mocks = vi.hoisted(() => ({ loadConfig: vi.fn(), resolveSessionStoreTargets: vi.fn(), + resolveSessionStoreTargetsOrExit: vi.fn(), resolveMaintenanceConfig: vi.fn(), loadSessionStore: vi.fn(), resolveSessionFilePath: vi.fn(), @@ -21,6 +22,7 @@ vi.mock("../config/config.js", () => ({ vi.mock("./session-store-targets.js", () => ({ resolveSessionStoreTargets: mocks.resolveSessionStoreTargets, + resolveSessionStoreTargetsOrExit: mocks.resolveSessionStoreTargetsOrExit, })); vi.mock("../config/sessions.js", () => ({ @@ -55,6 +57,17 @@ describe("sessionsCleanupCommand", () => { mocks.resolveSessionStoreTargets.mockReturnValue([ { agentId: "main", storePath: "/resolved/sessions.json" }, ]); + mocks.resolveSessionStoreTargetsOrExit.mockImplementation( + (params: { cfg: unknown; opts: unknown; runtime: RuntimeEnv }) => { + try { + return mocks.resolveSessionStoreTargets(params.cfg, params.opts); + } catch (error) { + params.runtime.error(error instanceof Error ? error.message : String(error)); + params.runtime.exit(1); + return null; + } + }, + ); mocks.resolveMaintenanceConfig.mockReturnValue({ mode: "warn", pruneAfterMs: 7 * 24 * 60 * 60 * 1000, diff --git a/src/commands/sessions-cleanup.ts b/src/commands/sessions-cleanup.ts index 151fa531e..a0b1d0723 100644 --- a/src/commands/sessions-cleanup.ts +++ b/src/commands/sessions-cleanup.ts @@ -14,7 +14,10 @@ import { } from "../config/sessions.js"; import type { RuntimeEnv } from "../runtime.js"; import { isRich, theme } from "../terminal/theme.js"; -import { resolveSessionStoreTargets, type SessionStoreTarget } from "./session-store-targets.js"; +import { + resolveSessionStoreTargetsOrExit, + type SessionStoreTarget, +} from "./session-store-targets.js"; import { formatSessionAgeCell, formatSessionFlagsCell, @@ -291,16 +294,16 @@ export async function sessionsCleanupCommand(opts: SessionsCleanupOptions, runti const cfg = loadConfig(); const displayDefaults = resolveSessionDisplayDefaults(cfg); const mode = opts.enforce ? "enforce" : resolveMaintenanceConfig().mode; - let targets: SessionStoreTarget[]; - try { - targets = resolveSessionStoreTargets(cfg, { + const targets = resolveSessionStoreTargetsOrExit({ + cfg, + opts: { store: opts.store, agent: opts.agent, allAgents: opts.allAgents, - }); - } catch (error) { - runtime.error(error instanceof Error ? error.message : String(error)); - runtime.exit(1); + }, + runtime, + }); + if (!targets) { return; } diff --git a/src/commands/sessions.ts b/src/commands/sessions.ts index 1615bf022..b72dfbe98 100644 --- a/src/commands/sessions.ts +++ b/src/commands/sessions.ts @@ -7,7 +7,7 @@ import { info } from "../globals.js"; import { parseAgentSessionKey } from "../routing/session-key.js"; import type { RuntimeEnv } from "../runtime.js"; import { isRich, theme } from "../terminal/theme.js"; -import { resolveSessionStoreTargets } from "./session-store-targets.js"; +import { resolveSessionStoreTargetsOrExit } from "./session-store-targets.js"; import { formatSessionAgeCell, formatSessionFlagsCell, @@ -95,16 +95,16 @@ export async function sessionsCommand( cfg.agents?.defaults?.contextTokens ?? lookupContextTokens(displayDefaults.model) ?? DEFAULT_CONTEXT_TOKENS; - let targets: ReturnType; - try { - targets = resolveSessionStoreTargets(cfg, { + const targets = resolveSessionStoreTargetsOrExit({ + cfg, + opts: { store: opts.store, agent: opts.agent, allAgents: opts.allAgents, - }); - } catch (error) { - runtime.error(error instanceof Error ? error.message : String(error)); - runtime.exit(1); + }, + runtime, + }); + if (!targets) { return; }