From 6dbc6cee3016d73f63d4622d96f167255be9ac43 Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Mon, 27 Apr 2026 11:01:55 -0500 Subject: [PATCH] fix(coven): close review edge cases --- extensions/coven/src/client.test.ts | 11 ++++ extensions/coven/src/client.ts | 3 + extensions/coven/src/config.test.ts | 20 +++++++ extensions/coven/src/config.ts | 3 + extensions/coven/src/runtime.test.ts | 87 +++++++++++++++++++++------- extensions/coven/src/runtime.ts | 30 ++++++---- 6 files changed, 122 insertions(+), 32 deletions(-) diff --git a/extensions/coven/src/client.test.ts b/extensions/coven/src/client.test.ts index 4f660659fa4..bb771a31f12 100644 --- a/extensions/coven/src/client.test.ts +++ b/extensions/coven/src/client.test.ts @@ -97,4 +97,15 @@ describe("createCovenClient", () => { /must not be a symlink/, ); }); + + it("rejects a socket root that resolves through a symlink", async () => { + const realHome = path.join(tmpDir, "real-coven"); + const symlinkHome = path.join(tmpDir, "symlink-coven"); + await fs.mkdir(realHome); + await fs.symlink(realHome, symlinkHome); + + await expect( + createCovenClient(path.join(symlinkHome, "coven.sock"), { socketRoot: symlinkHome }).health(), + ).rejects.toThrow(/covenHome must not be a symlink/); + }); }); diff --git a/extensions/coven/src/client.ts b/extensions/coven/src/client.ts index 03f159ad995..efc25ae420e 100644 --- a/extensions/coven/src/client.ts +++ b/extensions/coven/src/client.ts @@ -101,6 +101,9 @@ function validateSocketPathForUse(socketPath: string, socketRoot: string | undef if (!socketRoot) { return; } + if (lstatIfExists(socketRoot)?.isSymbolicLink()) { + throw new Error("Coven covenHome must not be a symlink"); + } const socketStat = lstatIfExists(socketPath); if (socketStat?.isSymbolicLink()) { throw new Error("Coven socketPath must not be a symlink"); diff --git a/extensions/coven/src/config.test.ts b/extensions/coven/src/config.test.ts index 88ff25a7ee0..e249ee2854e 100644 --- a/extensions/coven/src/config.test.ts +++ b/extensions/coven/src/config.test.ts @@ -75,6 +75,26 @@ describe("resolveCovenPluginConfig", () => { } }); + it("rejects covenHome when it is a symlink", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-coven-config-")); + const realHome = path.join(workspaceDir, "real-coven"); + const symlinkHome = path.join(workspaceDir, "symlink-coven"); + await fs.mkdir(realHome); + await fs.symlink(realHome, symlinkHome); + try { + expect(() => + resolveCovenPluginConfig({ + rawConfig: { + covenHome: symlinkHome, + }, + workspaceDir, + }), + ).toThrow(/covenHome must not be a symlink/); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }); + } + }); + it("uses COVEN_HOME with tilde expansion for the default socket path", () => { process.env.COVEN_HOME = "~/.custom-coven"; diff --git a/extensions/coven/src/config.ts b/extensions/coven/src/config.ts index 3ff813dede8..cdc4db88aa8 100644 --- a/extensions/coven/src/config.ts +++ b/extensions/coven/src/config.ts @@ -93,6 +93,9 @@ function resolveCovenHome(raw: string | undefined, baseDir: string): string { } function resolveSocketPath(covenHome: string, raw: string | undefined, baseDir: string): string { + if (lstatIfExists(covenHome)?.isSymbolicLink()) { + throw new Error("Coven covenHome must not be a symlink"); + } const socketPath = raw?.trim() ? resolveConfiguredPath(raw, baseDir) : path.join(covenHome, "coven.sock"); diff --git a/extensions/coven/src/runtime.test.ts b/extensions/coven/src/runtime.test.ts index 1ea13b3f734..e1f9215fb8d 100644 --- a/extensions/coven/src/runtime.test.ts +++ b/extensions/coven/src/runtime.test.ts @@ -8,24 +8,41 @@ import { type AcpRuntimeEvent, type AcpRuntimeHandle, } from "openclaw/plugin-sdk/acp-runtime"; -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { CovenClient, CovenEventRecord, CovenSessionRecord } from "./client.js"; import type { ResolvedCovenPluginConfig } from "./config.js"; import { __testing, CovenAcpRuntime } from "./runtime.js"; -const config: ResolvedCovenPluginConfig = { - covenHome: "/tmp/coven", - socketPath: "/tmp/coven/coven.sock", - workspaceDir: "/repo", +const baseConfig: ResolvedCovenPluginConfig = { + covenHome: "", + socketPath: "", + workspaceDir: "", fallbackBackend: "acpx", pollIntervalMs: 1, harnesses: {}, }; +let workspaceDir: string; +let config: ResolvedCovenPluginConfig; + +beforeEach(async () => { + workspaceDir = await fs.realpath( + await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-coven-workspace-")), + ); + const covenHome = path.join(workspaceDir, ".coven"); + await fs.mkdir(covenHome); + config = { + ...baseConfig, + covenHome, + socketPath: path.join(covenHome, "coven.sock"), + workspaceDir, + }; +}); + function session(overrides: Partial = {}): CovenSessionRecord { return { id: "session-1", - projectRoot: "/repo", + projectRoot: workspaceDir, harness: "codex", title: "Fix tests", status: "running", @@ -79,7 +96,7 @@ function fallbackRuntime(): AcpRuntime { sessionKey: "agent:codex:test", backend: "acpx", runtimeSessionName: "fallback-session", - cwd: "/repo", + cwd: workspaceDir, }; return { ensureSession: vi.fn(async () => handle), @@ -96,6 +113,7 @@ function fallbackRuntime(): AcpRuntime { afterEach(() => { vi.useRealTimers(); unregisterAcpRuntimeBackend("acpx"); + return fs.rm(workspaceDir, { recursive: true, force: true }); }); describe("CovenAcpRuntime", () => { @@ -115,7 +133,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); expect(handle.backend).toBe("acpx"); @@ -142,7 +160,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); await vi.advanceTimersByTimeAsync(5_000); const handle = await pending; @@ -157,7 +175,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); const events = await collect( @@ -171,8 +189,8 @@ describe("CovenAcpRuntime", () => { expect(client.launchSession).toHaveBeenCalledWith( expect.objectContaining({ - projectRoot: "/repo", - cwd: "/repo", + projectRoot: workspaceDir, + cwd: workspaceDir, harness: "codex", prompt: "Fix tests", }), @@ -194,7 +212,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); handle.runtimeSessionName = __testing.encodeRuntimeSessionName({ agent: "codex", @@ -213,8 +231,8 @@ describe("CovenAcpRuntime", () => { expect(client.launchSession).toHaveBeenCalledWith( expect.objectContaining({ - projectRoot: "/repo", - cwd: "/repo", + projectRoot: workspaceDir, + cwd: workspaceDir, }), undefined, ); @@ -226,7 +244,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); handle.cwd = "/tmp/attacker"; @@ -300,7 +318,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); await collect( @@ -329,7 +347,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); const events = await collect( @@ -347,7 +365,7 @@ describe("CovenAcpRuntime", () => { it("strips terminal escape and control characters from Coven output", () => { expect( __testing.sanitizeTerminalText( - "\u001b]0;spoof\u0007hi\u001b[31m!\u001b[0m\u001b7\u001bc\r\n", + "\u001b]0;spoof\u0007hi\u001b[31m!\u001b[0m\u001b7\u001bc\u202e\r\n", ), ).toBe("hi!\n"); }); @@ -377,6 +395,35 @@ describe("CovenAcpRuntime", () => { expect(__testing.decodeRuntimeSessionName(`coven:${"a".repeat(2_049)}`)).toBeNull(); }); + it("rejects missing Coven cwd paths before launching", async () => { + const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-coven-workspace-")); + try { + const runtime = new CovenAcpRuntime({ + config: { ...config, workspaceDir }, + client: fakeClient(), + }); + const handle = await runtime.ensureSession({ + sessionKey: "agent:codex:test", + agent: "codex", + mode: "oneshot", + cwd: path.join(workspaceDir, "missing"), + }); + + await expect( + collect( + runtime.runTurn({ + handle, + text: "Fix tests", + mode: "prompt", + requestId: "req-1", + }), + ), + ).rejects.toThrow(/outside workspace/); + } finally { + await fs.rm(workspaceDir, { recursive: true, force: true }); + } + }); + it("preserves direct fallback when Coven launch fails after detection", async () => { const fallback = fallbackRuntime(); registerAcpRuntimeBackend({ id: "acpx", runtime: fallback }); @@ -392,7 +439,7 @@ describe("CovenAcpRuntime", () => { sessionKey: "agent:codex:test", agent: "codex", mode: "oneshot", - cwd: "/repo", + cwd: workspaceDir, }); const events = await collect( diff --git a/extensions/coven/src/runtime.ts b/extensions/coven/src/runtime.ts index 76f50597236..550451af344 100644 --- a/extensions/coven/src/runtime.ts +++ b/extensions/coven/src/runtime.ts @@ -58,8 +58,12 @@ function encodeRuntimeSessionName(state: CovenRuntimeSessionState): string { } function decodeRuntimeSessionName(value: string): CovenRuntimeSessionState | null { - const encoded = value.startsWith("coven:") ? value.slice("coven:".length) : ""; - if (!encoded || encoded.length > MAX_RUNTIME_SESSION_NAME_BYTES) { + const prefix = "coven:"; + if (!value.startsWith(prefix) || value.length > prefix.length + MAX_RUNTIME_SESSION_NAME_BYTES) { + return null; + } + const encoded = value.slice(prefix.length); + if (!encoded) { return null; } try { @@ -125,6 +129,7 @@ const c0UnitSeparator = String.fromCharCode(0x1f); const del = String.fromCharCode(0x7f); const c1Start = String.fromCharCode(0x80); const c1End = String.fromCharCode(0x9f); +const BIDI_CONTROL_REGEX = /\p{Bidi_Control}/gu; const ANSI_ESCAPE_REGEX = new RegExp( `${ESC}(?:\\][\\s\\S]*?(?:${BEL}|${ESC}\\\\)|P[\\s\\S]*?${ESC}\\\\|\\[[\\x20-\\x3f]*[\\x40-\\x7e]|[\\x20-\\x2f]*[\\x30-\\x7e])`, "g", @@ -135,7 +140,10 @@ const TEXT_CONTROL_REGEX = new RegExp( ); function sanitizeTerminalText(input: string): string { - return input.replace(ANSI_ESCAPE_REGEX, "").replace(TEXT_CONTROL_REGEX, ""); + return input + .replace(ANSI_ESCAPE_REGEX, "") + .replace(TEXT_CONTROL_REGEX, "") + .replace(BIDI_CONTROL_REGEX, ""); } function sanitizeStatusText(input: string): string { @@ -296,7 +304,7 @@ export class CovenAcpRuntime implements AcpRuntime { let lastSeenEventId: string | undefined; while (true) { if (input.signal?.aborted) { - await this.killActiveSession(session.id, input.signal).catch(() => undefined); + await this.killActiveSession(session.id).catch(() => undefined); throw input.signal.reason ?? new Error("Coven turn aborted"); } @@ -341,7 +349,7 @@ export class CovenAcpRuntime implements AcpRuntime { } } catch (error) { if (input.signal?.aborted) { - await this.killActiveSession(session.id, input.signal).catch(() => undefined); + await this.killActiveSession(session.id).catch(() => undefined); throw input.signal.reason ?? error; } this.logger?.warn(`coven polling failed: ${String(error)}`); @@ -381,9 +389,9 @@ export class CovenAcpRuntime implements AcpRuntime { backendSessionId: session.id, agentSessionId: session.id, details: { - projectRoot: session.projectRoot, - harness: session.harness, - status: session.status, + projectRoot: sanitizeStatusText(session.projectRoot), + harness: sanitizeStatusText(session.harness), + status: sanitizeStatusText(session.status), exitCode: session.exitCode, }, }; @@ -510,12 +518,10 @@ export class CovenAcpRuntime implements AcpRuntime { const cwd = path.resolve(candidate ?? this.config.workspaceDir); const workspaceReal = realpathIfExists(this.config.workspaceDir); const cwdReal = realpathIfExists(cwd); - const boundary = workspaceReal ?? this.config.workspaceDir; - const checkedCwd = cwdReal ?? cwd; - if (!pathIsInside(boundary, checkedCwd)) { + if (!workspaceReal || !cwdReal || !pathIsInside(workspaceReal, cwdReal)) { throw new AcpRuntimeError("ACP_SESSION_INIT_FAILED", "Coven cwd is outside workspace."); } - return checkedCwd; + return cwdReal; } private async killActiveSession(sessionId: string, signal?: AbortSignal): Promise {