fix(coven): close review edge cases

This commit is contained in:
Val Alexander
2026-04-27 11:01:55 -05:00
parent 97d38af353
commit 6dbc6cee30
6 changed files with 122 additions and 32 deletions

View File

@@ -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/);
});
});

View File

@@ -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");

View File

@@ -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";

View File

@@ -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");

View File

@@ -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> = {}): 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(

View File

@@ -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<void> {