mirror of https://github.com/openclaw/openclaw.git
refactor: tighten ACP spawn failure typing
This commit is contained in:
Peter Steinberger
parent
b167ad052c
commit
be4eb269fc
|
|
@ -99,12 +99,18 @@ const resolveAcpSpawnStreamLogPathSpy = vi.spyOn(
|
|||
const { spawnAcpDirect } = await import("./acp-spawn.js");
|
||||
type SpawnRequest = Parameters<typeof spawnAcpDirect>[0];
|
||||
type SpawnContext = Parameters<typeof spawnAcpDirect>[1];
|
||||
type SpawnResult = Awaited<ReturnType<typeof spawnAcpDirect>>;
|
||||
type AgentCallParams = {
|
||||
deliver?: boolean;
|
||||
channel?: string;
|
||||
to?: string;
|
||||
threadId?: string;
|
||||
};
|
||||
type CrossAgentWorkspaceFixture = {
|
||||
workspaceRoot: string;
|
||||
mainWorkspace: string;
|
||||
targetWorkspace: string;
|
||||
};
|
||||
|
||||
function replaceSpawnConfig(next: OpenClawConfig): void {
|
||||
const current = hoisted.state.cfg as Record<string, unknown>;
|
||||
|
|
@ -186,12 +192,68 @@ function createRequesterContext(overrides?: Partial<SpawnContext>): SpawnContext
|
|||
};
|
||||
}
|
||||
|
||||
async function createCrossAgentWorkspaceFixture(options?: {
|
||||
targetDirName?: string;
|
||||
createTargetWorkspace?: boolean;
|
||||
}): Promise<CrossAgentWorkspaceFixture> {
|
||||
const workspaceRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-acp-spawn-"));
|
||||
const mainWorkspace = path.join(workspaceRoot, "main");
|
||||
const targetWorkspace = path.join(workspaceRoot, options?.targetDirName?.trim() || "claude-code");
|
||||
await fs.mkdir(mainWorkspace, { recursive: true });
|
||||
if (options?.createTargetWorkspace !== false) {
|
||||
await fs.mkdir(targetWorkspace, { recursive: true });
|
||||
}
|
||||
return {
|
||||
workspaceRoot,
|
||||
mainWorkspace,
|
||||
targetWorkspace,
|
||||
};
|
||||
}
|
||||
|
||||
function configureCrossAgentWorkspaceSpawn(fixture: CrossAgentWorkspaceFixture): void {
|
||||
replaceSpawnConfig({
|
||||
...hoisted.state.cfg,
|
||||
acp: {
|
||||
...hoisted.state.cfg.acp,
|
||||
allowedAgents: ["codex", "claude-code"],
|
||||
},
|
||||
agents: {
|
||||
list: [
|
||||
{
|
||||
id: "main",
|
||||
default: true,
|
||||
workspace: fixture.mainWorkspace,
|
||||
},
|
||||
{
|
||||
id: "claude-code",
|
||||
workspace: fixture.targetWorkspace,
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
function findAgentGatewayCall(): { method?: string; params?: Record<string, unknown> } | undefined {
|
||||
return hoisted.callGatewayMock.mock.calls
|
||||
.map((call: unknown[]) => call[0] as { method?: string; params?: Record<string, unknown> })
|
||||
.find((request) => request.method === "agent");
|
||||
}
|
||||
|
||||
function expectFailedSpawn(
|
||||
result: SpawnResult,
|
||||
status?: "error" | "forbidden",
|
||||
): Extract<SpawnResult, { status: "error" | "forbidden" }> {
|
||||
if (status) {
|
||||
expect(result.status).toBe(status);
|
||||
} else {
|
||||
expect(result.status).not.toBe("accepted");
|
||||
}
|
||||
if (result.status === "accepted") {
|
||||
throw new Error("Expected ACP spawn to fail");
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
function expectAgentGatewayCall(overrides: AgentCallParams): void {
|
||||
const agentCall = findAgentGatewayCall();
|
||||
expect(agentCall?.params?.deliver).toBe(overrides.deliver);
|
||||
|
|
@ -542,33 +604,9 @@ describe("spawnAcpDirect", () => {
|
|||
});
|
||||
|
||||
it("uses the target agent workspace for cross-agent ACP spawns when cwd is omitted", async () => {
|
||||
const workspaceRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-acp-spawn-"));
|
||||
const fixture = await createCrossAgentWorkspaceFixture();
|
||||
try {
|
||||
const mainWorkspace = path.join(workspaceRoot, "main");
|
||||
const targetWorkspace = path.join(workspaceRoot, "claude-code");
|
||||
await fs.mkdir(mainWorkspace, { recursive: true });
|
||||
await fs.mkdir(targetWorkspace, { recursive: true });
|
||||
|
||||
replaceSpawnConfig({
|
||||
...hoisted.state.cfg,
|
||||
acp: {
|
||||
...hoisted.state.cfg.acp,
|
||||
allowedAgents: ["codex", "claude-code"],
|
||||
},
|
||||
agents: {
|
||||
list: [
|
||||
{
|
||||
id: "main",
|
||||
default: true,
|
||||
workspace: mainWorkspace,
|
||||
},
|
||||
{
|
||||
id: "claude-code",
|
||||
workspace: targetWorkspace,
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
configureCrossAgentWorkspaceSpawn(fixture);
|
||||
|
||||
const result = await spawnAcpDirect(
|
||||
{
|
||||
|
|
@ -586,41 +624,21 @@ describe("spawnAcpDirect", () => {
|
|||
expect.objectContaining({
|
||||
sessionKey: expect.stringMatching(/^agent:claude-code:acp:/),
|
||||
agent: "claude-code",
|
||||
cwd: targetWorkspace,
|
||||
cwd: fixture.targetWorkspace,
|
||||
}),
|
||||
);
|
||||
} finally {
|
||||
await fs.rm(workspaceRoot, { recursive: true, force: true });
|
||||
await fs.rm(fixture.workspaceRoot, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("falls back to backend default cwd when the inherited target workspace does not exist", async () => {
|
||||
const workspaceRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-acp-spawn-"));
|
||||
const fixture = await createCrossAgentWorkspaceFixture({
|
||||
targetDirName: "claude-code-missing",
|
||||
createTargetWorkspace: false,
|
||||
});
|
||||
try {
|
||||
const mainWorkspace = path.join(workspaceRoot, "main");
|
||||
const missingTargetWorkspace = path.join(workspaceRoot, "claude-code-missing");
|
||||
await fs.mkdir(mainWorkspace, { recursive: true });
|
||||
|
||||
replaceSpawnConfig({
|
||||
...hoisted.state.cfg,
|
||||
acp: {
|
||||
...hoisted.state.cfg.acp,
|
||||
allowedAgents: ["codex", "claude-code"],
|
||||
},
|
||||
agents: {
|
||||
list: [
|
||||
{
|
||||
id: "main",
|
||||
default: true,
|
||||
workspace: mainWorkspace,
|
||||
},
|
||||
{
|
||||
id: "claude-code",
|
||||
workspace: missingTargetWorkspace,
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
configureCrossAgentWorkspaceSpawn(fixture);
|
||||
|
||||
const result = await spawnAcpDirect(
|
||||
{
|
||||
|
|
@ -642,39 +660,15 @@ describe("spawnAcpDirect", () => {
|
|||
}),
|
||||
);
|
||||
} finally {
|
||||
await fs.rm(workspaceRoot, { recursive: true, force: true });
|
||||
await fs.rm(fixture.workspaceRoot, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("surfaces non-missing target workspace access failures instead of silently dropping cwd", async () => {
|
||||
const workspaceRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-acp-spawn-"));
|
||||
const fixture = await createCrossAgentWorkspaceFixture();
|
||||
const accessSpy = vi.spyOn(fs, "access");
|
||||
try {
|
||||
const mainWorkspace = path.join(workspaceRoot, "main");
|
||||
const targetWorkspace = path.join(workspaceRoot, "claude-code");
|
||||
await fs.mkdir(mainWorkspace, { recursive: true });
|
||||
await fs.mkdir(targetWorkspace, { recursive: true });
|
||||
|
||||
replaceSpawnConfig({
|
||||
...hoisted.state.cfg,
|
||||
acp: {
|
||||
...hoisted.state.cfg.acp,
|
||||
allowedAgents: ["codex", "claude-code"],
|
||||
},
|
||||
agents: {
|
||||
list: [
|
||||
{
|
||||
id: "main",
|
||||
default: true,
|
||||
workspace: mainWorkspace,
|
||||
},
|
||||
{
|
||||
id: "claude-code",
|
||||
workspace: targetWorkspace,
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
configureCrossAgentWorkspaceSpawn(fixture);
|
||||
|
||||
accessSpy.mockRejectedValueOnce(
|
||||
Object.assign(new Error("permission denied"), { code: "EACCES" }),
|
||||
|
|
@ -693,12 +687,13 @@ describe("spawnAcpDirect", () => {
|
|||
|
||||
expect(result).toEqual({
|
||||
status: "error",
|
||||
errorCode: "cwd_resolution_failed",
|
||||
error: "permission denied",
|
||||
});
|
||||
expect(hoisted.initializeSessionMock).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
accessSpy.mockRestore();
|
||||
await fs.rm(workspaceRoot, { recursive: true, force: true });
|
||||
await fs.rm(fixture.workspaceRoot, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
|
|
@ -1063,8 +1058,7 @@ describe("spawnAcpDirect", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(result.status).toBe("error");
|
||||
expect(result.error).toContain("set `acp.defaultAgent`");
|
||||
expect(expectFailedSpawn(result, "error").error).toContain("set `acp.defaultAgent`");
|
||||
});
|
||||
|
||||
it("fails fast when Discord ACP thread spawn is disabled", async () => {
|
||||
|
|
@ -1094,8 +1088,7 @@ describe("spawnAcpDirect", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(result.status).toBe("error");
|
||||
expect(result.error).toContain("spawnAcpSessions=true");
|
||||
expect(expectFailedSpawn(result, "error").error).toContain("spawnAcpSessions=true");
|
||||
});
|
||||
|
||||
it("forbids ACP spawn from sandboxed requester sessions", async () => {
|
||||
|
|
@ -1118,8 +1111,9 @@ describe("spawnAcpDirect", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(result.status).toBe("forbidden");
|
||||
expect(result.error).toContain("Sandboxed sessions cannot spawn ACP sessions");
|
||||
expect(expectFailedSpawn(result, "forbidden").error).toContain(
|
||||
"Sandboxed sessions cannot spawn ACP sessions",
|
||||
);
|
||||
expect(hoisted.callGatewayMock).not.toHaveBeenCalled();
|
||||
expect(hoisted.initializeSessionMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
|
@ -1136,8 +1130,7 @@ describe("spawnAcpDirect", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(result.status).toBe("forbidden");
|
||||
expect(result.error).toContain('sandbox="require"');
|
||||
expect(expectFailedSpawn(result, "forbidden").error).toContain('sandbox="require"');
|
||||
expect(hoisted.callGatewayMock).not.toHaveBeenCalled();
|
||||
expect(hoisted.initializeSessionMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
|
@ -1653,8 +1646,7 @@ describe("spawnAcpDirect", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(result.status).toBe("error");
|
||||
expect(result.error).toContain("agent dispatch failed");
|
||||
expect(expectFailedSpawn(result, "error").error).toContain("agent dispatch failed");
|
||||
expect(relayHandle.dispose).toHaveBeenCalledTimes(1);
|
||||
expect(relayHandle.notifyStarted).not.toHaveBeenCalled();
|
||||
});
|
||||
|
|
@ -1673,8 +1665,7 @@ describe("spawnAcpDirect", () => {
|
|||
},
|
||||
);
|
||||
|
||||
expect(result.status).toBe("error");
|
||||
expect(result.error).toContain('streamTo="parent"');
|
||||
expect(expectFailedSpawn(result, "error").error).toContain('streamTo="parent"');
|
||||
expect(hoisted.callGatewayMock).not.toHaveBeenCalled();
|
||||
expect(hoisted.startAcpSpawnParentStreamRelayMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -94,16 +94,38 @@ export type SpawnAcpContext = {
|
|||
sandboxed?: boolean;
|
||||
};
|
||||
|
||||
export type SpawnAcpResult = {
|
||||
status: "accepted" | "forbidden" | "error";
|
||||
export const ACP_SPAWN_ERROR_CODES = [
|
||||
"acp_disabled",
|
||||
"requester_session_required",
|
||||
"runtime_policy",
|
||||
"thread_required",
|
||||
"target_agent_required",
|
||||
"agent_forbidden",
|
||||
"cwd_resolution_failed",
|
||||
"thread_binding_invalid",
|
||||
"spawn_failed",
|
||||
"dispatch_failed",
|
||||
] as const;
|
||||
export type SpawnAcpErrorCode = (typeof ACP_SPAWN_ERROR_CODES)[number];
|
||||
|
||||
type SpawnAcpAcceptedResult = {
|
||||
status: "accepted";
|
||||
childSessionKey?: string;
|
||||
runId?: string;
|
||||
mode?: SpawnAcpMode;
|
||||
streamLogPath?: string;
|
||||
note?: string;
|
||||
error?: string;
|
||||
};
|
||||
|
||||
type SpawnAcpFailedResult = {
|
||||
status: "forbidden" | "error";
|
||||
childSessionKey?: string;
|
||||
error: string;
|
||||
errorCode: SpawnAcpErrorCode;
|
||||
};
|
||||
|
||||
export type SpawnAcpResult = SpawnAcpAcceptedResult | SpawnAcpFailedResult;
|
||||
|
||||
export const ACP_SPAWN_ACCEPTED_NOTE =
|
||||
"initial ACP task queued in isolated session; follow-ups continue in the bound thread.";
|
||||
export const ACP_SPAWN_SESSION_ACCEPTED_NOTE =
|
||||
|
|
@ -369,6 +391,25 @@ function summarizeError(err: unknown): string {
|
|||
return "error";
|
||||
}
|
||||
|
||||
function createAcpSpawnFailure(params: {
|
||||
status: "forbidden" | "error";
|
||||
errorCode: SpawnAcpErrorCode;
|
||||
error: string;
|
||||
childSessionKey?: string;
|
||||
}): SpawnAcpFailedResult {
|
||||
return {
|
||||
status: params.status,
|
||||
errorCode: params.errorCode,
|
||||
error: params.error,
|
||||
...(params.childSessionKey ? { childSessionKey: params.childSessionKey } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
function isMissingPathError(error: unknown): boolean {
|
||||
const code = error instanceof Error ? (error as NodeJS.ErrnoException).code : undefined;
|
||||
return code === "ENOENT" || code === "ENOTDIR";
|
||||
}
|
||||
|
||||
async function resolveRuntimeCwdForAcpSpawn(params: {
|
||||
resolvedCwd?: string;
|
||||
explicitCwd?: string;
|
||||
|
|
@ -383,8 +424,7 @@ async function resolveRuntimeCwdForAcpSpawn(params: {
|
|||
await fs.access(params.resolvedCwd);
|
||||
return params.resolvedCwd;
|
||||
} catch (error) {
|
||||
const code = error instanceof Error ? (error as NodeJS.ErrnoException).code : undefined;
|
||||
if (code === "ENOENT" || code === "ENOTDIR") {
|
||||
if (isMissingPathError(error)) {
|
||||
return undefined;
|
||||
}
|
||||
throw error;
|
||||
|
|
@ -870,18 +910,20 @@ export async function spawnAcpDirect(
|
|||
requesterSessionKey: ctx.agentSessionKey,
|
||||
});
|
||||
if (!isAcpEnabledByPolicy(cfg)) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "forbidden",
|
||||
errorCode: "acp_disabled",
|
||||
error: "ACP is disabled by policy (`acp.enabled=false`).",
|
||||
};
|
||||
});
|
||||
}
|
||||
const streamToParentRequested = params.streamTo === "parent";
|
||||
const parentSessionKey = ctx.agentSessionKey?.trim();
|
||||
if (streamToParentRequested && !parentSessionKey) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: "requester_session_required",
|
||||
error: 'sessions_spawn streamTo="parent" requires an active requester session context.',
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
let requestThreadBinding = params.thread === true;
|
||||
|
|
@ -892,10 +934,11 @@ export async function spawnAcpDirect(
|
|||
sandbox: params.sandbox,
|
||||
});
|
||||
if (runtimePolicyError) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "forbidden",
|
||||
errorCode: "runtime_policy",
|
||||
error: runtimePolicyError,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
const spawnMode = resolveSpawnMode({
|
||||
|
|
@ -903,10 +946,11 @@ export async function spawnAcpDirect(
|
|||
threadRequested: requestThreadBinding,
|
||||
});
|
||||
if (spawnMode === "session" && !requestThreadBinding) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: "thread_required",
|
||||
error: 'mode="session" requires thread=true so the ACP session can stay bound to a thread.',
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
const requesterState = resolveAcpSpawnRequesterState({
|
||||
|
|
@ -926,18 +970,20 @@ export async function spawnAcpDirect(
|
|||
cfg,
|
||||
});
|
||||
if (!targetAgentResult.ok) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: "target_agent_required",
|
||||
error: targetAgentResult.error,
|
||||
};
|
||||
});
|
||||
}
|
||||
const targetAgentId = targetAgentResult.agentId;
|
||||
const agentPolicyError = resolveAcpAgentPolicyError(cfg, targetAgentId);
|
||||
if (agentPolicyError) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "forbidden",
|
||||
errorCode: "agent_forbidden",
|
||||
error: agentPolicyError.message,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
const sessionKey = `agent:${targetAgentId}:acp:${crypto.randomUUID()}`;
|
||||
|
|
@ -948,10 +994,19 @@ export async function spawnAcpDirect(
|
|||
requesterSessionKey: ctx.agentSessionKey,
|
||||
explicitWorkspaceDir: params.cwd,
|
||||
});
|
||||
const runtimeCwd = await resolveRuntimeCwdForAcpSpawn({
|
||||
resolvedCwd,
|
||||
explicitCwd: params.cwd,
|
||||
});
|
||||
let runtimeCwd: string | undefined;
|
||||
try {
|
||||
runtimeCwd = await resolveRuntimeCwdForAcpSpawn({
|
||||
resolvedCwd,
|
||||
explicitCwd: params.cwd,
|
||||
});
|
||||
} catch (error) {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: "cwd_resolution_failed",
|
||||
error: summarizeError(error),
|
||||
});
|
||||
}
|
||||
|
||||
let preparedBinding: PreparedAcpThreadBinding | null = null;
|
||||
if (requestThreadBinding) {
|
||||
|
|
@ -964,10 +1019,11 @@ export async function spawnAcpDirect(
|
|||
groupId: ctx.agentGroupId,
|
||||
});
|
||||
if (!prepared.ok) {
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: "thread_binding_invalid",
|
||||
error: prepared.error,
|
||||
};
|
||||
});
|
||||
}
|
||||
preparedBinding = prepared.binding;
|
||||
}
|
||||
|
|
@ -1014,10 +1070,11 @@ export async function spawnAcpDirect(
|
|||
deleteTranscript: true,
|
||||
runtimeCloseHandle: initializedRuntime,
|
||||
});
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: isSessionBindingError(err) ? "thread_binding_invalid" : "spawn_failed",
|
||||
error: isSessionBindingError(err) ? err.message : summarizeError(err),
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
const deliveryPlan = resolveAcpSpawnBootstrapDeliveryPlan({
|
||||
|
|
@ -1075,11 +1132,12 @@ export async function spawnAcpDirect(
|
|||
shouldDeleteSession: true,
|
||||
deleteTranscript: true,
|
||||
});
|
||||
return {
|
||||
return createAcpSpawnFailure({
|
||||
status: "error",
|
||||
errorCode: "dispatch_failed",
|
||||
error: summarizeError(err),
|
||||
childSessionKey: sessionKey,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
if (effectiveStreamToParent && parentSessionKey) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue