diff --git a/config/knip.config.ts b/config/knip.config.ts index baa140751ed6..92107b7450b9 100644 --- a/config/knip.config.ts +++ b/config/knip.config.ts @@ -247,6 +247,8 @@ const rootEntries = [ // OpenGrep rule tests read these as static source inputs; they are never executed. "security/opengrep/rules/ghsa-82g8-464f-2mv7/skill-env.js!", "security/opengrep/rules/ghsa-82g8-464f-2mv7/skill-env.ts!", + "security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.js!", + "security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.ts!", "openclaw.mjs!", "src/index.ts!", "src/entry.ts!", diff --git a/docs/plugins/codex-harness-runtime/sandbox-streaming.md b/docs/plugins/codex-harness-runtime/sandbox-streaming.md index a8ef81dc4f58..63115b64e405 100644 --- a/docs/plugins/codex-harness-runtime/sandbox-streaming.md +++ b/docs/plugins/codex-harness-runtime/sandbox-streaming.md @@ -19,6 +19,10 @@ polling and replay, so long-running processes cannot grow the app-server bridge without limit. Process exit and cleanup remain tied to the sandbox-owned process. Failed environment registration never falls back to host execution. +Interactive commands receive a real terminal. Ctrl-C interrupts both interactive +and noninteractive native commands. Closing the exec-server connection cancels its +outstanding HTTP requests and waits for cleanup before releasing the sandbox lease. + See [Sandboxed native execution](/plugins/codex-harness-reference#sandboxed-native-execution) for configuration and local-only transport restrictions. diff --git a/docs/plugins/sdk-runtime/config-and-utilities.md b/docs/plugins/sdk-runtime/config-and-utilities.md index e0b6ea87b4a1..cb9a56ceff44 100644 --- a/docs/plugins/sdk-runtime/config-and-utilities.md +++ b/docs/plugins/sdk-runtime/config-and-utilities.md @@ -89,6 +89,15 @@ session reservation or temporary output, await `withCommandProcessScope` from th same subpath around execution before releasing those resources. The scope joins late startup and process cleanup; uncertain cleanup remains an error. +Interactive process adapters can use `spawnTerminalPty` from the same subpath. +It owns platform-specific terminal creation, including the Node helper on Bun. +Pass the caller's construction signal and current-authority check through its +second argument. The caller owns output subscriptions, termination, and waiting +for the terminal's exit before releasing its backend resources. + +Sandbox command adapters retain the sandbox owner's per-stream output bound, +`SANDBOX_COMMAND_MAX_BUFFER_BYTES`, from `openclaw/plugin-sdk/sandbox`. + `WorkerTaskPool` from `openclaw/plugin-sdk/process-runtime` retains workers and unconsumed inputs when termination fails. Retry `close()` on that same pool; dispose dependent files only after closure is acknowledged. The optional diff --git a/extensions/codex/src/app-server/attempt-startup-computer-use.test.ts b/extensions/codex/src/app-server/attempt-startup-computer-use.test.ts index e5abb4a698f9..5e6f259a8f47 100644 --- a/extensions/codex/src/app-server/attempt-startup-computer-use.test.ts +++ b/extensions/codex/src/app-server/attempt-startup-computer-use.test.ts @@ -1,33 +1,46 @@ -// Exercises Computer Use readiness through the real Codex attempt startup owner. import fs from "node:fs/promises"; import { fileURLToPath } from "node:url"; import { AgentHarnessPreflightError } from "openclaw/plugin-sdk/agent-harness-runtime"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { answerInitialize, + createAttemptPaths, createAttemptThreadStarter, readHarnessRequestMethods, waitForRequest, } from "./attempt-startup.test-support.js"; -import { threadStartResult } from "./codex-app-server.test-fixtures.js"; -import type { CodexPluginConfig } from "./config.js"; +import { CodexAppServerClient } from "./client.js"; +import { threadStartResult as createThreadStartResult } from "./codex-app-server.test-fixtures.js"; +import { readCodexComputerUseStatus } from "./computer-use.js"; +import { createComputerUseRequest, requireRecord } from "./computer-use.test-support.js"; +import { resolveCodexAppServerRuntimeOptions, type CodexPluginConfig } from "./config.js"; import { setManagedCodexPluginRoot } from "./managed-binary.js"; import { defaultCodexPluginMetadataCache } from "./plugin-metadata-cache.js"; import { resetCodexTestBindingStore } from "./session-binding.test-helpers.js"; -import { clearSharedCodexAppServerClientAndWait } from "./shared-client.js"; +import { + clearSharedCodexAppServerClientAndWait, + getLeasedSharedCodexAppServerClient, + releaseLeasedSharedCodexAppServerClient, +} from "./shared-client.js"; import { createInferenceReadyClientHarness } from "./test-support.js"; +import { CODEX_APP_SERVER_VERSION } from "./version.js"; + +vi.mock("./desktop-generation.js", () => ({ + isCodexDesktopGenerationCurrent: () => false, + waitForCodexDesktopGeneration: async () => undefined, +})); const tempRoots = new Set(); const pluginConfig: CodexPluginConfig = { appServer: { command: "codex" } }; const startThreadWithHarness = createAttemptThreadStarter(tempRoots, pluginConfig); +const threadStartResult = (threadId = "thread-1") => createThreadStartResult(threadId, "/repo"); -describe("Codex attempt Computer Use readiness", () => { +describe("Computer Use attempt startup", () => { beforeEach(async () => { vi.useRealTimers(); vi.stubEnv("CODEX_API_KEY", ""); vi.stubEnv("OPENAI_API_KEY", ""); await clearSharedCodexAppServerClientAndWait(); - // Direct runtime tests supply the plugin root normally owned by loader registration. setManagedCodexPluginRoot(fileURLToPath(new URL("../../", import.meta.url))); defaultCodexPluginMetadataCache.clear(); resetCodexTestBindingStore(); @@ -46,6 +59,139 @@ describe("Codex attempt Computer Use readiness", () => { tempRoots.clear(); }); + it("retries one-off status after its client rejects a stale desktop selection", async () => { + const original = createStatusClient(); + const replacement = createStatusClient(); + const start = vi + .spyOn(CodexAppServerClient, "start") + .mockResolvedValueOnce(original.client) + .mockResolvedValueOnce(replacement.client); + const paths = createAttemptPaths(tempRoots); + const statusPluginConfig = { + ...pluginConfig, + computerUse: { enabled: true, marketplaceName: "desktop-tools" }, + } satisfies CodexPluginConfig; + const runtime = resolveCodexAppServerRuntimeOptions({ pluginConfig: statusPluginConfig }); + const firstLease = await getLeasedSharedCodexAppServerClient({ + startOptions: runtime.start, + pluginConfig: statusPluginConfig, + agentDir: paths.agentDir, + }); + const staleGuard = vi.fn(async () => { + throw Object.assign(new Error("desktop selection changed"), { + code: "CODEX_APP_SERVER_START_SELECTION_CHANGED", + }); + }); + firstLease.setThreadSessionRequestGuard(staleGuard); + releaseLeasedSharedCodexAppServerClient(firstLease); + + await expect( + readCodexComputerUseStatus({ pluginConfig: statusPluginConfig, agentDir: paths.agentDir }), + ).resolves.toMatchObject({ ready: true }); + expect(staleGuard).toHaveBeenCalledTimes(1); + expect(start).toHaveBeenCalledTimes(2); + expect(original.stdinDestroyed).toBe(true); + expect(readHarnessRequestMethods(original)).not.toContain("thread/start"); + expect(readHarnessRequestMethods(original)).not.toContain("mcpServer/tool/call"); + expect(readHarnessRequestMethods(original)).not.toContain("thread/unsubscribe"); + expect( + readHarnessRequestMethods(replacement).filter((method) => + ["thread/start", "mcpServer/tool/call", "thread/unsubscribe"].includes(method ?? ""), + ), + ).toEqual(["thread/start", "mcpServer/tool/call", "thread/unsubscribe"]); + const probe = await waitForRequest(replacement, "mcpServer/tool/call"); + const cleanup = await waitForRequest(replacement, "thread/unsubscribe"); + expect(cleanup.params).toEqual({ + threadId: requireRecord(probe.params, "readiness probe").threadId, + }); + }); + + it("charges initial client acquisition to the one-off status deadline", async () => { + let elapsedMs = 0; + vi.spyOn(performance, "now").mockImplementation(() => elapsedMs); + vi.spyOn(Date, "now").mockImplementation(() => elapsedMs); + const harness = createStatusClient((method) => { + if (method === "initialize") { + elapsedMs = 800; + } else if (method === "mcpServerStatus/list") { + elapsedMs = 1_050; + } + }); + vi.spyOn(CodexAppServerClient, "start").mockResolvedValueOnce(harness.client); + const requests = vi.spyOn(harness.client, "request"); + const paths = createAttemptPaths(tempRoots); + const status = await readCodexComputerUseStatus({ + agentDir: paths.agentDir, + timeoutMs: 1_000, + pluginConfig: { + ...pluginConfig, + computerUse: { enabled: true, marketplaceName: "desktop-tools" }, + }, + }); + expect(status.ready).toBe(false); + expect(requests).toHaveBeenCalledWith( + "plugin/list", + expect.anything(), + expect.objectContaining({ timeoutMs: 200 }), + ); + expect(readHarnessRequestMethods(harness)).not.toContain("thread/start"); + expect(readHarnessRequestMethods(harness)).not.toContain("mcpServer/tool/call"); + }); + + it.each(["discovery", "probe"] as const)( + "honors the one-off status deadline while preserving cleanup after %s", + async (completedPhase) => { + let elapsedMs = 0; + vi.spyOn(performance, "now").mockImplementation(() => elapsedMs); + vi.spyOn(Date, "now").mockImplementation(() => elapsedMs); + const expiresAfter = + completedPhase === "discovery" ? "mcpServerStatus/list" : "mcpServer/tool/call"; + const harness = createStatusClient((method) => { + if (method === expiresAfter) { + elapsedMs = 2_000; + } + }); + vi.spyOn(CodexAppServerClient, "start").mockResolvedValueOnce(harness.client); + const requests = vi.spyOn(harness.client, "request"); + const paths = createAttemptPaths(tempRoots); + const status = await readCodexComputerUseStatus({ + agentDir: paths.agentDir, + timeoutMs: 1_000, + pluginConfig: { + ...pluginConfig, + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + liveTestTimeoutMs: 1_500, + toolCallTimeoutMs: 100, + }, + }, + }); + expect(status.ready).toBe(completedPhase === "probe"); + if (completedPhase === "discovery") { + expect(readHarnessRequestMethods(harness)).not.toContain("thread/start"); + expect(readHarnessRequestMethods(harness)).not.toContain("mcpServer/tool/call"); + } else { + expect(requests).toHaveBeenCalledWith( + "thread/unsubscribe", + { threadId: "computer-use-probe-thread-1" }, + expect.objectContaining({ timeoutMs: 1_500, signal: expect.any(AbortSignal) }), + ); + expect(harness.stdinDestroyed).toBe(false); + expect(requests).toHaveBeenCalledWith( + "thread/start", + expect.anything(), + expect.objectContaining({ timeoutMs: 1_000 }), + ); + expect(requests).toHaveBeenCalledWith( + "mcpServer/tool/call", + expect.anything(), + expect.objectContaining({ timeoutMs: 100 }), + ); + } + }, + ); + it.each([false, true])( "does not await optional live probes at turn startup (strictReadiness: %s)", async (strictReadiness) => { @@ -83,8 +229,7 @@ describe("Codex attempt Computer Use readiness", () => { send({ id: request.id, result: threadStartResult() }); break; case "thread/unsubscribe": - case "thread/archive": - send({ id: request.id, result: {} }); + send({ id: request.id, result: { status: "unsubscribed" } }); break; // Leave mcpServer/tool/call unanswered to exercise the real 60s timeout. } @@ -138,7 +283,7 @@ describe("Codex attempt Computer Use readiness", () => { }); expect(userStarts).toHaveLength(0); expect( - readHarnessRequestMethods(harness).filter((method) => method === "thread/archive"), + readHarnessRequestMethods(harness).filter((method) => method === "thread/unsubscribe"), ).toHaveLength(2); } else { await vi.waitFor(() => expect(settled).toBe(true), { interval: 1, timeout: 1_000 }); @@ -150,7 +295,33 @@ describe("Codex attempt Computer Use readiness", () => { result?.turnRoute.release(); result?.releaseSharedClientLease(); } + expect(readHarnessRequestMethods(harness)).not.toContain("thread/archive"); expect(readHarnessRequestMethods(harness)).not.toContain("config/mcpServer/reload"); }, ); }); + +function createStatusClient(afterResponse?: (method: string) => void) { + const fixture = createComputerUseRequest({ installed: true }); + return createInferenceReadyClientHarness({ + onWrite(line, send) { + const frame = JSON.parse(line) as { id?: number; method: string; params?: unknown }; + if (frame.id === undefined) { + return; + } + const response = + frame.method === "initialize" + ? Promise.resolve({ userAgent: `codex-cli/${CODEX_APP_SERVER_VERSION}` }) + : frame.method === "configRequirements/read" + ? Promise.resolve({ requirements: null }) + : fixture(frame.method, frame.params); + void response.then( + (result) => { + send({ id: frame.id, result: result ?? null }); + afterResponse?.(frame.method); + }, + (error: unknown) => send({ id: frame.id, error: { code: -32000, message: String(error) } }), + ); + }, + }); +} diff --git a/extensions/codex/src/app-server/computer-use-health.test.ts b/extensions/codex/src/app-server/computer-use-health.test.ts index e873c1be1bcf..9c2c91ea0e7d 100644 --- a/extensions/codex/src/app-server/computer-use-health.test.ts +++ b/extensions/codex/src/app-server/computer-use-health.test.ts @@ -224,7 +224,7 @@ function createClient(options: { liveTestFailures?: number } = {}) { if (method === "config/mcpServer/reload") { return undefined; } - if (method === "thread/unsubscribe" || method === "thread/archive") { + if (method === "thread/unsubscribe") { expect(params).toEqual({ threadId: `health-probe-thread-${threadStarts}` }); return undefined; } diff --git a/extensions/codex/src/app-server/computer-use-health.ts b/extensions/codex/src/app-server/computer-use-health.ts index 7f888ec1d474..f6f0f00590a1 100644 --- a/extensions/codex/src/app-server/computer-use-health.ts +++ b/extensions/codex/src/app-server/computer-use-health.ts @@ -2,7 +2,7 @@ import { embeddedAgentLog } from "openclaw/plugin-sdk/agent-harness-runtime"; import { defineCodexBuildState } from "../build-state.js"; import type { CodexAppServerClient } from "./client.js"; -import { runCodexComputerUseLiveTest } from "./computer-use.js"; +import { runCodexComputerUseLiveTest } from "./computer-use-readiness.js"; import type { ResolvedCodexComputerUseConfig } from "./config.js"; type ComputerUseHealthMonitor = { @@ -92,15 +92,17 @@ async function runCodexComputerUseHealthProbe( monitor.running = true; try { const { liveTest, repair } = await runCodexComputerUseLiveTest({ + client, config, tools, request: async ( method: string, requestParams?: unknown, - requestOptions?: { timeoutMs?: number }, + requestOptions?: { timeoutMs?: number; signal?: AbortSignal }, ) => await client.request(method, requestParams, { timeoutMs: requestOptions?.timeoutMs ?? config.liveTestTimeoutMs, + ...(requestOptions?.signal ? { signal: requestOptions.signal } : {}), }), }); if (!liveTest.ok) { diff --git a/extensions/codex/src/app-server/computer-use-readiness.test.ts b/extensions/codex/src/app-server/computer-use-readiness.test.ts new file mode 100644 index 000000000000..d03ba1f5a4a6 --- /dev/null +++ b/extensions/codex/src/app-server/computer-use-readiness.test.ts @@ -0,0 +1,615 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + runCodexComputerUseLiveTest, + type CodexComputerUseRequest, +} from "./computer-use-readiness.js"; +import { + ensureCodexComputerUse, + installCodexComputerUse, + readCodexComputerUseStatus, +} from "./computer-use.js"; +import { + createComputerUseRequest, + expectRequestMethodNotCalled, + expectSetupErrorStatus, + expectStatusFields, + requestCalls, + requireRecord, +} from "./computer-use.test-support.js"; +import { resolveCodexComputerUseConfig } from "./config.js"; +import { createClientHarness, waitForHarnessRequest } from "./test-support.js"; + +const sharedClientMocks = vi.hoisted(() => ({ + getLeasedSharedCodexAppServerClient: vi.fn(), + releaseLeasedSharedCodexAppServerClient: vi.fn(), +})); + +vi.mock("./shared-client.js", async (importOriginal) => ({ + ...(await importOriginal()), + ...sharedClientMocks, +})); + +describe("Codex Computer Use readiness", () => { + afterEach(() => { + vi.useRealTimers(); + sharedClientMocks.getLeasedSharedCodexAppServerClient.mockReset(); + sharedClientMocks.releaseLeasedSharedCodexAppServerClient.mockReset(); + }); + + it("unsubscribes a cancelled readiness probe while another request completes", async () => { + const fixture = createComputerUseRequest({ installed: true }); + const controller = new AbortController(); + let probeSubscribed = false; + const harness = createClientHarness({ + onWrite(line, send) { + const frame = JSON.parse(line) as { id: number; method: string; params?: unknown }; + if (frame.method === "turn/start" || frame.method === "mcpServer/tool/call") { + return; + } + void fixture(frame.method, frame.params).then( + (result) => { + if (frame.method === "thread/start") { + probeSubscribed = true; + } else if (frame.method === "thread/unsubscribe") { + probeSubscribed = false; + } + send({ id: frame.id, result: result ?? null }); + }, + (error: unknown) => + send({ + id: frame.id, + error: { code: -32000, message: String(error) }, + }), + ); + }, + }); + const peer = harness.client.request("turn/start", { threadId: "healthy-peer" }); + void peer.catch(() => undefined); + try { + const peerStart = await waitForHarnessRequest(harness, "turn/start"); + const readiness = ensureCodexComputerUse({ + client: harness.client, + pluginConfig: { + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + strictReadiness: true, + liveTestTimeoutMs: 1_000, + }, + }, + signal: controller.signal, + }); + const rejected = expect(readiness).rejects.toThrow("aborted"); + await waitForHarnessRequest(harness, "mcpServer/tool/call"); + expect(probeSubscribed).toBe(true); + + controller.abort(); + await rejected; + + expect(probeSubscribed).toBe(false); + expect(harness.stdinDestroyed).toBe(false); + harness.send({ id: peerStart.id, result: { turn: { id: "healthy-turn" } } }); + await expect(peer).resolves.toEqual({ turn: { id: "healthy-turn" } }); + } finally { + await harness.client.closeAndWait(); + } + }); + + it.each(["passed", "cancelled", "mcp-error"] as const)( + "retires a probe client after failed unsubscribe (probe: %s)", + async (probe) => { + const cancelled = probe === "cancelled"; + const fixture = createComputerUseRequest({ + installed: true, + liveTestFailures: probe === "mcp-error" ? 1 : 0, + }); + const controller = new AbortController(); + const harness = createClientHarness({ + onWrite(line, send) { + const frame = JSON.parse(line) as { id: number; method: string; params?: unknown }; + if (frame.method === "mcpServer/tool/call" && cancelled) { + return; + } + if (frame.method === "thread/unsubscribe") { + send({ id: frame.id, error: { code: -32000, message: "unsubscribe failed" } }); + return; + } + void fixture(frame.method, frame.params).then( + (result) => send({ id: frame.id, result: result ?? null }), + (error: unknown) => + send({ id: frame.id, error: { code: -32000, message: String(error) } }), + ); + }, + }); + try { + const readiness = ensureCodexComputerUse({ + client: harness.client, + pluginConfig: { + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + autoRepair: true, + strictReadiness: true, + }, + }, + signal: controller.signal, + }); + const rejected = expect(readiness).rejects.toThrow( + cancelled ? "aborted" : "Computer Use readiness cleanup failed", + ); + if (cancelled) { + await waitForHarnessRequest(harness, "mcpServer/tool/call"); + controller.abort(); + } + await rejected; + expect(harness.stdinDestroyed).toBe(true); + expect(requestCalls(fixture).filter(([method]) => method === "thread/start")).toHaveLength( + 1, + ); + } finally { + await harness.client.closeAndWait(); + } + }, + ); + + it("holds one client lease through a one-off readiness probe and its cleanup", async () => { + const fixture = createComputerUseRequest({ installed: true }); + const harness = createClientHarness({ + onWrite(line, send) { + const frame = JSON.parse(line) as { id: number; method: string; params?: unknown }; + void fixture(frame.method, frame.params).then( + (result) => send({ id: frame.id, result: result ?? null }), + (error: unknown) => + send({ id: frame.id, error: { code: -32000, message: String(error) } }), + ); + }, + }); + sharedClientMocks.getLeasedSharedCodexAppServerClient.mockResolvedValue(harness.client); + try { + await expect( + readCodexComputerUseStatus({ + pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, + }), + ).resolves.toMatchObject({ ready: true }); + expect(sharedClientMocks.getLeasedSharedCodexAppServerClient).toHaveBeenCalledTimes(1); + expect( + sharedClientMocks.releaseLeasedSharedCodexAppServerClient, + ).toHaveBeenCalledExactlyOnceWith(harness.client); + expect(fixture).toHaveBeenCalledWith("thread/unsubscribe", { + threadId: "computer-use-probe-thread-1", + }); + } finally { + await harness.client.closeAndWait(); + } + }); + + it("reports an installed Computer Use MCP server from a registered marketplace", async () => { + const request = createComputerUseRequest({ installed: true }); + + const status = await readCodexComputerUseStatus({ + pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, + request, + }); + + expectStatusFields(status, { + enabled: true, + ready: true, + reason: "ready", + installed: true, + pluginEnabled: true, + mcpServerAvailable: true, + marketplaceName: "desktop-tools", + tools: ["list_apps"], + message: "Computer Use is ready.", + }); + expect(status.installation).toMatchObject({ + status: "installed", + ok: true, + }); + expect(status.exposure).toMatchObject({ + status: "available", + ok: true, + }); + expect(status.liveTest).toMatchObject({ + status: "passed", + ok: true, + attempted: true, + attempts: 1, + timeoutMs: 60_000, + retried: false, + repaired: false, + }); + expect(request).toHaveBeenCalledWith( + "thread/start", + { + input: [], + developerInstructions: "OpenClaw Computer Use readiness probe", + ephemeral: true, + }, + { timeoutMs: 60_000 }, + ); + expect(request).toHaveBeenCalledWith( + "mcpServer/tool/call", + { + threadId: "computer-use-probe-thread-1", + server: "computer-use", + tool: "list_apps", + arguments: {}, + }, + { + timeoutMs: 60_000, + }, + ); + expect(request).toHaveBeenCalledWith( + "thread/unsubscribe", + { threadId: "computer-use-probe-thread-1" }, + { timeoutMs: 60_000, signal: expect.any(AbortSignal) }, + ); + expectRequestMethodNotCalled(request, "thread/archive"); + expectRequestMethodNotCalled(request, "marketplace/add"); + expectRequestMethodNotCalled(request, "experimentalFeature/enablement/set"); + expectRequestMethodNotCalled(request, "plugin/install"); + }); + + it("probes unified Computer Use through its JavaScript tool", async () => { + const request = createComputerUseRequest({ + installed: true, + pluginName: "unified-computer-use", + mcpServerName: "cua_repl", + mcpTools: ["js", "js_reset", "turn_ended"], + }); + + const status = await readCodexComputerUseStatus({ + pluginConfig: { + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + pluginName: "unified-computer-use", + mcpServerName: "cua_repl", + }, + }, + request, + }); + + expect(request).toHaveBeenCalledWith( + "mcpServer/tool/call", + { + threadId: "computer-use-probe-thread-1", + server: "cua_repl", + tool: "js", + arguments: { code: "await cua.getState();" }, + }, + { timeoutMs: 60_000 }, + ); + expectStatusFields(status, { + ready: true, + reason: "ready", + pluginName: "unified-computer-use", + mcpServerName: "cua_repl", + tools: ["js", "js_reset", "turn_ended"], + }); + }); + + it("inherits managed security policy when starting a Computer Use readiness probe", async () => { + const request = createComputerUseRequest({ installed: true }); + const managedRequest = vi.fn(async (method: string, params?: unknown) => { + if (method === "thread/start") { + const threadParams = requireRecord(params, "managed readiness thread"); + if ("sandbox" in threadParams || "approvalPolicy" in threadParams) { + throw new Error("enterprise policy does not permit thread security overrides"); + } + } + return await request(method, params); + }) as CodexComputerUseRequest; + + const status = await readCodexComputerUseStatus({ + pluginConfig: { + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + strictReadiness: true, + }, + }, + request: managedRequest, + }); + + expect(status).toMatchObject({ ready: true, reason: "ready" }); + }); + + it("treats MCP error results as failed readiness probes", async () => { + const request = createComputerUseRequest({ installed: true, liveTestResultErrors: 2 }); + + const status = await readCodexComputerUseStatus({ + pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, + request, + }); + + expect(status).toMatchObject({ ready: false, reason: "live_test_failed" }); + expect(status.liveTest).toMatchObject({ + status: "failed", + ok: false, + attempts: 2, + error: "Computer Use readiness tool computer-use.list_apps returned an error result", + }); + }); + + it("repairs a failed probe through the owning MCP runtime without signaling sibling processes", async () => { + const request = createComputerUseRequest({ installed: true, liveTestFailures: 1 }); + const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true); + try { + const status = await readCodexComputerUseStatus({ + pluginConfig: { + computerUse: { enabled: true, marketplaceName: "desktop-tools", autoRepair: true }, + }, + request, + }); + + expect(status).toMatchObject({ ready: true, reason: "ready" }); + expect(status.liveTest).toMatchObject({ + status: "passed", + attempts: 2, + retried: true, + repaired: true, + }); + expect(status.repair).toMatchObject({ attempted: true, killedPids: [], warnings: [] }); + expect(request).toHaveBeenCalledWith("config/mcpServer/reload", undefined, { + timeoutMs: 60_000, + }); + const methods = requestCalls(request).map(([method]) => method); + expect(methods.indexOf("thread/unsubscribe")).toBeLessThan( + methods.indexOf("config/mcpServer/reload"), + ); + expect( + requestCalls(request).filter(([method]) => method === "mcpServer/tool/call"), + ).toHaveLength(2); + expect(killSpy).not.toHaveBeenCalled(); + } finally { + killSpy.mockRestore(); + } + }); + + it("reports an owner-managed MCP reload failure without preventing the bounded retry", async () => { + const request = createComputerUseRequest({ + installed: true, + liveTestFailures: 1, + reloadFailures: 1, + }); + + const status = await readCodexComputerUseStatus({ + pluginConfig: { + computerUse: { enabled: true, marketplaceName: "desktop-tools", autoRepair: true }, + }, + request, + }); + + expect(status.liveTest).toMatchObject({ status: "passed", attempts: 2, repaired: false }); + expect(status.repair).toMatchObject({ + attempted: true, + killedPids: [], + warnings: ["Could not reload Computer Use MCP servers: MCP runtime reload failed"], + }); + expect(status.warnings).toContain( + "Could not reload Computer Use MCP servers: MCP runtime reload failed", + ); + }); + + it.each([false, true])( + "propagates a desktop selection change without retrying the stale client (cleanup fails: %s)", + async (cleanupFails) => { + const selectionChanged = Object.assign(new Error("desktop selection changed"), { + code: "CODEX_APP_SERVER_START_SELECTION_CHANGED", + }); + const harness = createClientHarness(); + const request = vi.fn(async (method: string) => { + if (method === "thread/start") { + return { thread: { id: "probe-thread" } }; + } + if (method === "mcpServer/tool/call") { + throw selectionChanged; + } + if (method === "thread/unsubscribe") { + if (cleanupFails) { + throw new Error("unsubscribe failed"); + } + return undefined; + } + throw new Error(`unexpected request: ${method}`); + }) as CodexComputerUseRequest; + + try { + const failure = await runCodexComputerUseLiveTest({ + request, + client: harness.client, + config: resolveCodexComputerUseConfig({ + pluginConfig: { computerUse: { enabled: true, autoRepair: true } }, + }), + }).then( + () => undefined, + (error: unknown) => error, + ); + expect(harness.stdinDestroyed).toBe(cleanupFails); + expect(failure).toBe(selectionChanged); + expect(requestCalls(request).map(([method]) => method)).toEqual([ + "thread/start", + "mcpServer/tool/call", + "thread/unsubscribe", + ]); + } finally { + await harness.client.closeAndWait(); + } + }, + ); + + it.each([false, true])( + "fails fast when MCP exposes no tools (strict: %s)", + async (strictReadiness) => { + const request = createComputerUseRequest({ installed: true, mcpToolsAvailable: false }); + + await expectSetupErrorStatus( + ensureCodexComputerUse({ + pluginConfig: { + computerUse: { + enabled: true, + strictReadiness, + marketplaceName: "desktop-tools", + }, + }, + request, + }), + { + ready: false, + reason: "mcp_missing", + mcpServerAvailable: false, + tools: [], + message: "Computer Use is installed, but the computer-use MCP server exposes no tools.", + }, + ); + expectRequestMethodNotCalled(request, "thread/start"); + expectRequestMethodNotCalled(request, "mcpServer/tool/call"); + }, + ); + + it("reloads empty MCP exposure once during install before failing closed", async () => { + const request = createComputerUseRequest({ installed: true, mcpToolsAvailable: false }); + + await expectSetupErrorStatus( + installCodexComputerUse({ + pluginConfig: { computerUse: { marketplaceName: "desktop-tools" } }, + request, + }), + { ready: false, reason: "mcp_missing", mcpServerAvailable: false }, + ); + expect( + requestCalls(request).filter(([method]) => method === "config/mcpServer/reload"), + ).toHaveLength(1); + expectRequestMethodNotCalled(request, "thread/start"); + }); + + it("does not reload the Computer Use MCP runtime unless autoRepair is enabled", async () => { + const request = createComputerUseRequest({ installed: true, liveTestFailures: 2 }); + + const status = await readCodexComputerUseStatus({ + pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, + request, + }); + + expect(status.liveTest).toMatchObject({ + status: "failed", + ok: false, + attempts: 2, + retried: true, + repaired: false, + }); + expectStatusFields(status, { + ready: false, + reason: "live_test_failed", + installed: true, + pluginEnabled: true, + mcpServerAvailable: true, + }); + expect(status.warnings).toContain( + "Computer Use live test failed, but compatibility startup remains enabled; set computerUse.strictReadiness to true to fail closed.", + ); + expect(status.message).toContain( + "Startup is allowed because computerUse.strictReadiness is false.", + ); + expect(status.repair).toBeUndefined(); + expectRequestMethodNotCalled(request, "config/mcpServer/reload"); + }); + + it("surfaces install, exposure, and live-test layers separately when the live test fails", async () => { + const request = createComputerUseRequest({ installed: true, liveTestFailures: 2 }); + + const status = await readCodexComputerUseStatus({ + pluginConfig: { + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + autoRepair: true, + strictReadiness: true, + }, + }, + request, + }); + + expectStatusFields(status, { + ready: false, + reason: "live_test_failed", + installed: true, + pluginEnabled: true, + mcpServerAvailable: true, + }); + expect(status.installation).toMatchObject({ status: "installed", ok: true }); + expect(status.exposure).toMatchObject({ status: "available", ok: true }); + expect(status.liveTest).toMatchObject({ + status: "failed", + ok: false, + attempted: true, + attempts: 2, + timeoutMs: 60_000, + retried: true, + repaired: true, + error: "list_apps timed out", + }); + expect(status.message).toContain("Computer Use live test failed after 2 attempts"); + expect( + requestCalls(request).filter(([method]) => method === "config/mcpServer/reload"), + ).toHaveLength(1); + }); + + it.each([false, true])( + "skips live probes for non-strict startup (autoInstall: %s)", + async (autoInstall) => { + const request = createComputerUseRequest({ installed: !autoInstall, liveTestFailures: 2 }); + const status = await ensureCodexComputerUse({ + pluginConfig: { + computerUse: { enabled: true, autoInstall, marketplaceName: "desktop-tools" }, + }, + request, + }); + + expectStatusFields(status, { + ready: true, + reason: "ready", + installed: true, + pluginEnabled: true, + mcpServerAvailable: true, + }); + expect(status.liveTest).toMatchObject({ status: "skipped", ok: false, attempted: false }); + expectRequestMethodNotCalled(request, "thread/start"); + expectRequestMethodNotCalled(request, "mcpServer/tool/call"); + if (autoInstall) { + expect(request).toHaveBeenCalledWith("plugin/install", { + marketplacePath: "/marketplaces/desktop-tools/.agents/plugins/marketplace.json", + pluginName: "computer-use", + }); + } else { + expectRequestMethodNotCalled(request, "plugin/install"); + } + }, + ); + + it("fails startup closed when strictReadiness is enabled", async () => { + const request = createComputerUseRequest({ installed: true, liveTestFailures: 2 }); + + await expectSetupErrorStatus( + ensureCodexComputerUse({ + pluginConfig: { + computerUse: { + enabled: true, + marketplaceName: "desktop-tools", + strictReadiness: true, + }, + }, + request, + }), + { + ready: false, + reason: "live_test_failed", + installed: true, + pluginEnabled: true, + mcpServerAvailable: true, + }, + ); + }); +}); diff --git a/extensions/codex/src/app-server/computer-use-readiness.ts b/extensions/codex/src/app-server/computer-use-readiness.ts new file mode 100644 index 000000000000..31bd1e232936 --- /dev/null +++ b/extensions/codex/src/app-server/computer-use-readiness.ts @@ -0,0 +1,233 @@ +/** Computer Use readiness probes and their native subscription lifecycle. */ +import { toErrorObject } from "openclaw/plugin-sdk/error-runtime"; +import { + CodexAppServerUnsafeSubscriptionError, + retireUnsafeCodexTurnClientBestEffort, +} from "./attempt-client-cleanup.js"; +import { describeControlFailure } from "./capabilities.js"; +import type { CodexAppServerClient } from "./client.js"; +import type { ResolvedCodexComputerUseConfig } from "./config.js"; +import type { ToolCallResult as CodexMcpToolCallResult } from "./protocol-mcp.js"; +import type { CodexThreadStartResponse, JsonValue } from "./protocol.js"; +import { isCodexAppServerStartSelectionChangedError } from "./shared-client.js"; + +/** Minimal app-server request function needed by Computer Use setup. */ +export type CodexComputerUseRequest = ( + method: string, + params?: unknown, + options?: { timeoutMs?: number; signal?: AbortSignal }, +) => Promise; + +type CodexComputerUseLiveTestState = "skipped" | "passed" | "failed"; + +export type CodexComputerUseRepairStatus = { + attempted: boolean; + killedPids: number[]; + message: string; + warnings: string[]; +}; + +export type CodexComputerUseLiveTestStatus = { + status: CodexComputerUseLiveTestState; + ok: boolean; + attempted: boolean; + attempts: number; + timeoutMs: number; + retried: boolean; + repaired: boolean; + message: string; + error?: string; + durationMs?: number; +}; + +const COMPUTER_USE_LIVE_TEST_RETRY_COUNT = 1; +const COMPUTER_USE_LIVE_TEST_THREAD_NAME = "OpenClaw Computer Use readiness probe"; +const COMPUTER_USE_LIST_APPS_TOOL = "list_apps"; +const COMPUTER_USE_UNIFIED_JS_TOOL = "js"; +const COMPUTER_USE_UNIFIED_JS_PROBE = "await cua.getState();"; + +export async function runCodexComputerUseLiveTest(params: { + request: CodexComputerUseRequest; + client?: CodexAppServerClient; + signal?: AbortSignal; + config: ResolvedCodexComputerUseConfig; + tools?: readonly string[]; +}): Promise<{ liveTest: CodexComputerUseLiveTestStatus; repair?: CodexComputerUseRepairStatus }> { + const startedAt = Date.now(); + let lastError: unknown; + let repair: CodexComputerUseRepairStatus | undefined; + const probe = resolveComputerUseLiveTestProbe(params.tools); + for (let attempt = 0; attempt <= COMPUTER_USE_LIVE_TEST_RETRY_COUNT; attempt += 1) { + let threadId: string | undefined; + let outcome: + | { ok: true; liveTest: CodexComputerUseLiveTestStatus } + | { ok: false; error: unknown }; + try { + const thread = await params.request( + "thread/start", + { + input: [], + developerInstructions: COMPUTER_USE_LIVE_TEST_THREAD_NAME, + ephemeral: true, + }, + { + timeoutMs: params.config.liveTestTimeoutMs, + }, + ); + threadId = thread.thread.id; + const toolResult = await params.request( + "mcpServer/tool/call", + { + threadId, + server: params.config.mcpServerName, + tool: probe.tool, + arguments: probe.arguments, + }, + { + timeoutMs: params.config.toolCallTimeoutMs, + }, + ); + if (toolResult.isError === true) { + throw new Error( + `Computer Use readiness tool ${params.config.mcpServerName}.${probe.tool} returned an error result`, + ); + } + outcome = { + ok: true, + liveTest: { + status: "passed", + ok: true, + attempted: true, + attempts: attempt + 1, + timeoutMs: params.config.liveTestTimeoutMs, + retried: attempt > 0, + repaired: Boolean(repair?.attempted && repair.warnings.length === 0), + durationMs: Math.max(0, Date.now() - startedAt), + message: "Computer Use live test passed.", + }, + }; + } catch (error) { + outcome = { ok: false, error }; + } + let cleanupError: Error | undefined; + if (threadId) { + try { + await cleanupComputerUseProbeThread(params, threadId); + } catch (error) { + cleanupError = toErrorObject(error, "Computer Use readiness cleanup failed"); + } + } + if ( + !outcome.ok && + (params.signal?.aborted || isCodexAppServerStartSelectionChangedError(outcome.error)) + ) { + throw toErrorObject(outcome.error, "Computer Use live test failed"); + } + if (cleanupError) { + throw cleanupError; + } + if (outcome.ok) { + return { liveTest: outcome.liveTest, ...(repair ? { repair } : {}) }; + } + lastError = outcome.error; + if (attempt < COMPUTER_USE_LIVE_TEST_RETRY_COUNT && params.config.autoRepair) { + repair = await repairComputerUseMcpRuntime(params.request, params.config); + } + } + const errorMessage = describeControlFailure(lastError); + return { + liveTest: { + status: "failed", + ok: false, + attempted: true, + attempts: COMPUTER_USE_LIVE_TEST_RETRY_COUNT + 1, + timeoutMs: params.config.liveTestTimeoutMs, + retried: COMPUTER_USE_LIVE_TEST_RETRY_COUNT > 0, + repaired: Boolean(repair?.attempted && repair.warnings.length === 0), + durationMs: Math.max(0, Date.now() - startedAt), + message: `Computer Use live test failed after ${COMPUTER_USE_LIVE_TEST_RETRY_COUNT + 1} attempts: ${errorMessage}`, + error: errorMessage, + }, + ...(repair ? { repair } : {}), + }; +} + +function resolveComputerUseLiveTestProbe(tools: readonly string[] | undefined): { + tool: string; + arguments: Record; +} { + if ( + tools?.includes(COMPUTER_USE_UNIFIED_JS_TOOL) && + !tools.includes(COMPUTER_USE_LIST_APPS_TOOL) + ) { + return { + tool: COMPUTER_USE_UNIFIED_JS_TOOL, + arguments: { code: COMPUTER_USE_UNIFIED_JS_PROBE }, + }; + } + return { tool: COMPUTER_USE_LIST_APPS_TOOL, arguments: {} }; +} + +async function repairComputerUseMcpRuntime( + request: CodexComputerUseRequest, + config: ResolvedCodexComputerUseConfig, +): Promise { + try { + // Codex owns MCP process lifetimes; signaling descendants can kill an active sibling. + await request("config/mcpServer/reload", undefined, { timeoutMs: config.liveTestTimeoutMs }); + return { + attempted: true, + killedPids: [], + warnings: [], + message: "Reloaded Computer Use MCP servers through Codex app-server.", + }; + } catch (error) { + const message = `Could not reload Computer Use MCP servers: ${describeControlFailure(error)}`; + return { attempted: true, killedPids: [], warnings: [message], message }; + } +} + +async function cleanupComputerUseProbeThread( + params: { + request: CodexComputerUseRequest; + client?: CodexAppServerClient; + config: ResolvedCodexComputerUseConfig; + }, + threadId: string, +): Promise { + try { + // Ephemeral probes have no stored thread to archive. Release their subscription + // with a cleanup deadline that remains live after the caller is cancelled. + await params.request( + "thread/unsubscribe", + { threadId }, + { + timeoutMs: params.config.liveTestTimeoutMs, + signal: AbortSignal.timeout(params.config.liveTestTimeoutMs), + }, + ); + } catch (error) { + if (params.client) { + await retireUnsafeCodexTurnClientBestEffort(params.client, "Computer Use readiness cleanup"); + } + throw new CodexAppServerUnsafeSubscriptionError("Computer Use readiness cleanup failed", { + cause: error, + }); + } +} + +export function skippedLiveTestStatus( + config: ResolvedCodexComputerUseConfig, + message: string, +): CodexComputerUseLiveTestStatus { + return { + status: "skipped", + ok: false, + attempted: false, + attempts: 0, + timeoutMs: config.liveTestTimeoutMs, + retried: false, + repaired: false, + message, + }; +} diff --git a/extensions/codex/src/app-server/computer-use.test-support.ts b/extensions/codex/src/app-server/computer-use.test-support.ts new file mode 100644 index 000000000000..dbec886de46e --- /dev/null +++ b/extensions/codex/src/app-server/computer-use.test-support.ts @@ -0,0 +1,266 @@ +import { createRequireRecord } from "openclaw/plugin-sdk/test-fixtures"; +import { expect, vi } from "vitest"; +import type { CodexComputerUseRequest } from "./computer-use-readiness.js"; +import type { CodexComputerUseStatus } from "./computer-use.js"; + +export function expectStatusFields( + status: CodexComputerUseStatus, + fields: Partial, +): void { + for (const key of Object.keys(fields) as Array) { + expect(status[key]).toEqual(fields[key]); + } +} + +export async function expectSetupErrorStatus( + promise: Promise, + fields: Partial, +): Promise { + let caught: unknown; + try { + await promise; + } catch (error) { + caught = error; + } + const error = requireRecord(caught, "setup error"); + const status = requireRecord(error.status, "setup error status") as CodexComputerUseStatus; + expectStatusFields(status, fields); +} + +export const requireRecord = createRequireRecord("object", "label-not-object"); + +export function requestCalls( + request: CodexComputerUseRequest, +): ReadonlyArray { + return vi.mocked(request).mock.calls; +} + +export function expectRequestMethodNotCalled( + request: CodexComputerUseRequest, + method: string, +): void { + expect(requestCalls(request).map(([calledMethod]) => calledMethod)).not.toContain(method); +} + +export function createComputerUseRequest(params: { + installed: boolean; + enabled?: boolean; + pluginName?: string; + mcpServerName?: string; + mcpTools?: readonly string[]; + nativePluginsEnabled?: boolean | "absent"; + marketplaceAvailableAfterListCalls?: number; + liveTestFailures?: number; + liveTestResultErrors?: number; + reloadFailures?: number; + mcpToolsAvailable?: boolean; + remoteMarketplace?: { + name: string; + pluginId?: string | null; + }; + additionalMarketplaceNames?: readonly string[]; +}): CodexComputerUseRequest { + let installed = params.installed; + let enabled = params.enabled ?? installed; + let pluginListCalls = 0; + let liveTestFailures = params.liveTestFailures ?? 0; + let liveTestResultErrors = params.liveTestResultErrors ?? 0; + let reloadFailures = params.reloadFailures ?? 0; + let threadStartCalls = 0; + const pluginName = params.pluginName ?? "computer-use"; + const mcpServerName = params.mcpServerName ?? "computer-use"; + const mcpTools = params.mcpTools ?? ["list_apps"]; + const marketplaceName = params.remoteMarketplace?.name ?? "desktop-tools"; + const marketplacePath = params.remoteMarketplace + ? null + : `/marketplaces/${marketplaceName}/.agents/plugins/marketplace.json`; + const source = params.remoteMarketplace ? "remote" : "local"; + const currentPluginSummary = () => + pluginSummary( + installed, + marketplaceName, + enabled, + source, + params.remoteMarketplace?.pluginId, + pluginName, + ); + return vi.fn(async (method: string, requestParams?: unknown) => { + if (method === "experimentalFeature/enablement/set") { + return { + enablement: params.nativePluginsEnabled === false ? {} : { plugins: true }, + }; + } + if (method === "experimentalFeature/list") { + return { + data: + params.nativePluginsEnabled === "absent" + ? [] + : [{ name: "plugins", enabled: params.nativePluginsEnabled ?? true }], + nextCursor: null, + }; + } + if (method === "marketplace/add") { + return { + marketplaceName: "desktop-tools", + installedRoot: "/marketplaces/desktop-tools", + alreadyAdded: false, + }; + } + if (method === "plugin/list") { + pluginListCalls += 1; + const marketplaceAvailable = + pluginListCalls >= (params.marketplaceAvailableAfterListCalls ?? 1); + return { + marketplaces: marketplaceAvailable + ? [ + ...(params.additionalMarketplaceNames ?? []).map((name) => + marketplaceEntry(name, false), + ), + { + name: marketplaceName, + path: marketplacePath, + interface: null, + plugins: [currentPluginSummary()], + }, + ] + : [], + marketplaceLoadErrors: [], + featuredPluginIds: [], + }; + } + if (method === "plugin/read") { + expect(requestParams).toEqual( + params.remoteMarketplace + ? { + remoteMarketplaceName: marketplaceName, + pluginName: params.remoteMarketplace.pluginId, + } + : { marketplacePath, pluginName }, + ); + return { + plugin: { + marketplaceName, + marketplacePath, + summary: currentPluginSummary(), + description: "Control desktop apps.", + skills: [], + apps: [], + mcpServers: [mcpServerName], + }, + }; + } + if (method === "plugin/install") { + if (params.remoteMarketplace) { + expect(requestParams).toEqual({ + remoteMarketplaceName: marketplaceName, + pluginName: params.remoteMarketplace.pluginId, + }); + } + installed = true; + enabled = true; + return { authPolicy: "ON_INSTALL", appsNeedingAuth: [] }; + } + if (method === "config/mcpServer/reload") { + if (reloadFailures > 0) { + reloadFailures -= 1; + throw new Error("MCP runtime reload failed"); + } + return undefined; + } + if (method === "mcpServerStatus/list") { + return { + data: + installed && enabled + ? [ + { + name: mcpServerName, + tools: + params.mcpToolsAvailable === false + ? {} + : Object.fromEntries( + mcpTools.map((name) => [name, { name, inputSchema: { type: "object" } }]), + ), + resources: [], + resourceTemplates: [], + authStatus: "unsupported", + }, + ] + : [], + nextCursor: null, + }; + } + if (method === "thread/start") { + threadStartCalls += 1; + return { + thread: { + id: `computer-use-probe-thread-${threadStartCalls}`, + }, + model: "gpt-5.1", + modelProvider: "openai", + }; + } + if (method === "mcpServer/tool/call") { + const requestRecord = requireRecord(requestParams, "Computer Use readiness tool call"); + const tool = requestRecord.tool; + if (typeof tool !== "string" || !mcpTools.includes(tool)) { + return { + content: [{ type: "text", text: `Unknown tool: ${String(tool)}` }], + isError: true, + }; + } + expect(requestRecord).toEqual({ + threadId: `computer-use-probe-thread-${threadStartCalls}`, + server: mcpServerName, + tool, + arguments: tool === "js" ? { code: "await cua.getState();" } : {}, + }); + if (liveTestFailures > 0) { + liveTestFailures -= 1; + throw new Error(`${tool} timed out`); + } + if (liveTestResultErrors > 0) { + liveTestResultErrors -= 1; + return { content: [{ type: "text", text: `${tool} failed` }], isError: true }; + } + return { content: [{ type: "text", text: "[]" }] }; + } + if (method === "thread/unsubscribe") { + expect(requestParams).toEqual({ threadId: `computer-use-probe-thread-${threadStartCalls}` }); + return undefined; + } + throw new Error(`unexpected request ${method}`); + }) as CodexComputerUseRequest; +} + +export function marketplaceEntry(marketplaceName: string, installed: boolean) { + return { + name: marketplaceName, + path: `/marketplaces/${marketplaceName}/.agents/plugins/marketplace.json`, + interface: null, + plugins: [pluginSummary(installed, marketplaceName)], + }; +} + +export function pluginSummary( + installed: boolean, + marketplaceName = "desktop-tools", + enabled = installed, + source: "local" | "remote" = "local", + remotePluginId?: string | null, + pluginName = "computer-use", +) { + return { + id: `${pluginName}@${marketplaceName}`, + ...(source === "remote" ? { remotePluginId: remotePluginId ?? null } : {}), + name: pluginName, + source: + source === "local" + ? { type: "local", path: `/marketplaces/${marketplaceName}/plugins/${pluginName}` } + : { type: "remote" }, + installed, + enabled, + installPolicy: "AVAILABLE", + authPolicy: "ON_INSTALL", + interface: null, + }; +} diff --git a/extensions/codex/src/app-server/computer-use.test.ts b/extensions/codex/src/app-server/computer-use.test.ts index 1c51d53bdf85..61b1c21df991 100644 --- a/extensions/codex/src/app-server/computer-use.test.ts +++ b/extensions/codex/src/app-server/computer-use.test.ts @@ -1,14 +1,21 @@ import fs from "node:fs"; import path from "node:path"; // Codex tests cover computer use plugin behavior. -import { createRequireRecord } from "openclaw/plugin-sdk/test-fixtures"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { resolveCodexAppServerRuntimeOptions, resolveCodexComputerUseConfig } from "./config.js"; +import { + createComputerUseRequest, + expectRequestMethodNotCalled, + expectSetupErrorStatus, + expectStatusFields, + marketplaceEntry, + pluginSummary, + requestCalls, +} from "./computer-use.test-support.js"; +import { resolveCodexAppServerRuntimeOptions } from "./config.js"; import { acquireCodexNativeConfigFence } from "./native-config-fence.js"; import { resolveCodexNativeConfigFenceKey } from "./shared-client.js"; import { createClientHarness, useAutoCleanupTempDirTracker } from "./test-support.js"; -const requestCodexAppServerJsonMock = vi.hoisted(() => vi.fn()); const sharedClientMocks = vi.hoisted(() => ({ assertCodexAppServerClientStartSelectionCurrent: vi.fn(), getLeasedSharedCodexAppServerClient: vi.fn(), @@ -35,10 +42,6 @@ const managedProvisioningMocks = vi.hoisted(() => ({ ), })); -vi.mock("./request.js", () => ({ - requestCodexAppServerJson: requestCodexAppServerJsonMock, -})); - vi.mock("./shared-client.js", async (importOriginal) => ({ ...(await importOriginal()), ...sharedClientMocks, @@ -79,8 +82,6 @@ import { ensureCodexComputerUse, installCodexComputerUse, readCodexComputerUseStatus, - runCodexComputerUseLiveTest, - type CodexComputerUseStatus, } from "./computer-use.js"; type CodexComputerUseRequest = NonNullable< @@ -90,48 +91,11 @@ type CodexComputerUseRequest = NonNullable< const REMOTE_COMPUTER_USE_MARKETPLACE_NAME = "openai-curated-remote"; const REMOTE_COMPUTER_USE_PLUGIN_ID = "plugins~Plugin_00000000000000000000000000000000"; -function expectStatusFields( - status: CodexComputerUseStatus, - fields: Partial, -): void { - for (const key of Object.keys(fields) as Array) { - expect(status[key]).toEqual(fields[key]); - } -} - -async function expectSetupErrorStatus( - promise: Promise, - fields: Partial, -): Promise { - let caught: unknown; - try { - await promise; - } catch (error) { - caught = error; - } - const error = requireRecord(caught, "setup error"); - const status = requireRecord(error.status, "setup error status") as CodexComputerUseStatus; - expectStatusFields(status, fields); -} - -const requireRecord = createRequireRecord("object", "label-not-object"); - -function requestCalls( - request: CodexComputerUseRequest, -): ReadonlyArray { - return vi.mocked(request).mock.calls; -} - -function expectRequestMethodNotCalled(request: CodexComputerUseRequest, method: string): void { - expect(requestCalls(request).map(([calledMethod]) => calledMethod)).not.toContain(method); -} - describe("Codex Computer Use setup", () => { const tempDirs = useAutoCleanupTempDirTracker(afterEach); afterEach(() => { vi.useRealTimers(); - requestCodexAppServerJsonMock.mockReset(); sharedClientMocks.assertCodexAppServerClientStartSelectionCurrent.mockReset(); sharedClientMocks.getLeasedSharedCodexAppServerClient.mockReset(); sharedClientMocks.readCodexAppServerClientDesktopGeneration.mockReset(); @@ -162,14 +126,24 @@ describe("Codex Computer Use setup", () => { ); }); - it("stays disabled until configured", async () => { - const status = await readCodexComputerUseStatus({ pluginConfig: {}, request: vi.fn() }); + it.each([ + { name: "default", pluginConfig: {} }, + { + name: "disabled with invalid runtime", + pluginConfig: { computerUse: { enabled: false }, appServer: { command: "codex app-server" } }, + }, + ])("stays disabled before runtime resolution ($name)", async ({ pluginConfig }) => { + const status = await readCodexComputerUseStatus({ pluginConfig }); expectStatusFields(status, { enabled: false, ready: false, reason: "disabled", message: "Computer Use is disabled.", }); + await expect(ensureCodexComputerUse({ pluginConfig })).resolves.toMatchObject({ + reason: "disabled", + }); + expect(sharedClientMocks.getLeasedSharedCodexAppServerClient).not.toHaveBeenCalled(); }); it("starts one-off Computer Use setup with the desktop app owner", async () => { @@ -292,7 +266,6 @@ describe("Codex Computer Use setup", () => { await answer("thread/start"); await answer("mcpServer/tool/call"); await answer("thread/unsubscribe"); - await answer("thread/archive"); await expect(install).resolves.toMatchObject({ ready: true, @@ -404,426 +377,6 @@ describe("Codex Computer Use setup", () => { }, ); - it("reports an installed Computer Use MCP server from a registered marketplace", async () => { - const request = createComputerUseRequest({ installed: true }); - - const status = await readCodexComputerUseStatus({ - pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, - request, - }); - - expectStatusFields(status, { - enabled: true, - ready: true, - reason: "ready", - installed: true, - pluginEnabled: true, - mcpServerAvailable: true, - marketplaceName: "desktop-tools", - tools: ["list_apps"], - message: "Computer Use is ready.", - }); - expect(status.installation).toMatchObject({ - status: "installed", - ok: true, - }); - expect(status.exposure).toMatchObject({ - status: "available", - ok: true, - }); - expect(status.liveTest).toMatchObject({ - status: "passed", - ok: true, - attempted: true, - attempts: 1, - timeoutMs: 60_000, - retried: false, - repaired: false, - }); - expect(request).toHaveBeenCalledWith( - "thread/start", - { - input: [], - developerInstructions: "OpenClaw Computer Use readiness probe", - ephemeral: true, - }, - { timeoutMs: 60_000 }, - ); - expect(request).toHaveBeenCalledWith( - "mcpServer/tool/call", - { - threadId: "computer-use-probe-thread-1", - server: "computer-use", - tool: "list_apps", - arguments: {}, - }, - { - timeoutMs: 60_000, - }, - ); - expect(request).toHaveBeenCalledWith( - "thread/unsubscribe", - { threadId: "computer-use-probe-thread-1" }, - { timeoutMs: 60_000 }, - ); - expect(request).toHaveBeenCalledWith( - "thread/archive", - { threadId: "computer-use-probe-thread-1" }, - { timeoutMs: 60_000 }, - ); - expectRequestMethodNotCalled(request, "marketplace/add"); - expectRequestMethodNotCalled(request, "experimentalFeature/enablement/set"); - expectRequestMethodNotCalled(request, "plugin/install"); - }); - - it("probes unified Computer Use through its JavaScript tool", async () => { - const request = createComputerUseRequest({ - installed: true, - pluginName: "unified-computer-use", - mcpServerName: "cua_repl", - mcpTools: ["js", "js_reset", "turn_ended"], - }); - - const status = await readCodexComputerUseStatus({ - pluginConfig: { - computerUse: { - enabled: true, - marketplaceName: "desktop-tools", - pluginName: "unified-computer-use", - mcpServerName: "cua_repl", - }, - }, - request, - }); - - expect(request).toHaveBeenCalledWith( - "mcpServer/tool/call", - { - threadId: "computer-use-probe-thread-1", - server: "cua_repl", - tool: "js", - arguments: { code: "await cua.getState();" }, - }, - { timeoutMs: 60_000 }, - ); - expectStatusFields(status, { - ready: true, - reason: "ready", - pluginName: "unified-computer-use", - mcpServerName: "cua_repl", - tools: ["js", "js_reset", "turn_ended"], - }); - }); - - it("inherits managed security policy when starting a Computer Use readiness probe", async () => { - const request = createComputerUseRequest({ installed: true }); - const managedRequest = vi.fn(async (method: string, params?: unknown) => { - if (method === "thread/start") { - const threadParams = requireRecord(params, "managed readiness thread"); - if ("sandbox" in threadParams || "approvalPolicy" in threadParams) { - throw new Error("enterprise policy does not permit thread security overrides"); - } - } - return await request(method, params); - }) as CodexComputerUseRequest; - - const status = await readCodexComputerUseStatus({ - pluginConfig: { - computerUse: { - enabled: true, - marketplaceName: "desktop-tools", - strictReadiness: true, - }, - }, - request: managedRequest, - }); - - expect(status).toMatchObject({ ready: true, reason: "ready" }); - }); - - it("treats MCP error results as failed readiness probes", async () => { - const request = createComputerUseRequest({ installed: true, liveTestResultErrors: 2 }); - - const status = await readCodexComputerUseStatus({ - pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, - request, - }); - - expect(status).toMatchObject({ ready: false, reason: "live_test_failed" }); - expect(status.liveTest).toMatchObject({ - status: "failed", - ok: false, - attempts: 2, - error: "Computer Use readiness tool computer-use.list_apps returned an error result", - }); - }); - - it("repairs a failed probe through the owning MCP runtime without signaling sibling processes", async () => { - const request = createComputerUseRequest({ installed: true, liveTestFailures: 1 }); - const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true); - try { - const status = await readCodexComputerUseStatus({ - pluginConfig: { - computerUse: { enabled: true, marketplaceName: "desktop-tools", autoRepair: true }, - }, - request, - }); - - expect(status).toMatchObject({ ready: true, reason: "ready" }); - expect(status.liveTest).toMatchObject({ - status: "passed", - attempts: 2, - retried: true, - repaired: true, - }); - expect(status.repair).toMatchObject({ attempted: true, killedPids: [], warnings: [] }); - expect(request).toHaveBeenCalledWith("config/mcpServer/reload", undefined, { - timeoutMs: 60_000, - }); - const methods = requestCalls(request).map(([method]) => method); - expect(methods.indexOf("thread/archive")).toBeLessThan( - methods.indexOf("config/mcpServer/reload"), - ); - expect( - requestCalls(request).filter(([method]) => method === "mcpServer/tool/call"), - ).toHaveLength(2); - expect(killSpy).not.toHaveBeenCalled(); - } finally { - killSpy.mockRestore(); - } - }); - - it("reports an owner-managed MCP reload failure without preventing the bounded retry", async () => { - const request = createComputerUseRequest({ - installed: true, - liveTestFailures: 1, - reloadFailures: 1, - }); - - const status = await readCodexComputerUseStatus({ - pluginConfig: { - computerUse: { enabled: true, marketplaceName: "desktop-tools", autoRepair: true }, - }, - request, - }); - - expect(status.liveTest).toMatchObject({ status: "passed", attempts: 2, repaired: false }); - expect(status.repair).toMatchObject({ - attempted: true, - killedPids: [], - warnings: ["Could not reload Computer Use MCP servers: MCP runtime reload failed"], - }); - expect(status.warnings).toContain( - "Could not reload Computer Use MCP servers: MCP runtime reload failed", - ); - }); - - it("propagates a desktop selection change without retrying the stale client", async () => { - const selectionChanged = Object.assign(new Error("desktop selection changed"), { - code: "CODEX_APP_SERVER_START_SELECTION_CHANGED", - }); - const request = vi.fn(async (method: string) => { - if (method === "thread/start") { - return { thread: { id: "probe-thread" } }; - } - if (method === "mcpServer/tool/call") { - throw selectionChanged; - } - if (method === "thread/unsubscribe" || method === "thread/archive") { - return undefined; - } - throw new Error(`unexpected request: ${method}`); - }) as CodexComputerUseRequest; - - await expect( - runCodexComputerUseLiveTest({ - request, - config: resolveCodexComputerUseConfig({ - pluginConfig: { computerUse: { enabled: true, autoRepair: true } }, - }), - }), - ).rejects.toBe(selectionChanged); - expect(requestCalls(request).map(([method]) => method)).toEqual([ - "thread/start", - "mcpServer/tool/call", - "thread/unsubscribe", - "thread/archive", - ]); - }); - - it.each([false, true])( - "fails fast when MCP exposes no tools (strict: %s)", - async (strictReadiness) => { - const request = createComputerUseRequest({ installed: true, mcpToolsAvailable: false }); - - await expectSetupErrorStatus( - ensureCodexComputerUse({ - pluginConfig: { - computerUse: { - enabled: true, - strictReadiness, - marketplaceName: "desktop-tools", - }, - }, - request, - }), - { - ready: false, - reason: "mcp_missing", - mcpServerAvailable: false, - tools: [], - message: "Computer Use is installed, but the computer-use MCP server exposes no tools.", - }, - ); - expectRequestMethodNotCalled(request, "thread/start"); - expectRequestMethodNotCalled(request, "mcpServer/tool/call"); - }, - ); - - it("reloads empty MCP exposure once during install before failing closed", async () => { - const request = createComputerUseRequest({ installed: true, mcpToolsAvailable: false }); - - await expectSetupErrorStatus( - installCodexComputerUse({ - pluginConfig: { computerUse: { marketplaceName: "desktop-tools" } }, - request, - }), - { ready: false, reason: "mcp_missing", mcpServerAvailable: false }, - ); - expect( - requestCalls(request).filter(([method]) => method === "config/mcpServer/reload"), - ).toHaveLength(1); - expectRequestMethodNotCalled(request, "thread/start"); - }); - - it("does not reload the Computer Use MCP runtime unless autoRepair is enabled", async () => { - const request = createComputerUseRequest({ installed: true, liveTestFailures: 2 }); - - const status = await readCodexComputerUseStatus({ - pluginConfig: { computerUse: { enabled: true, marketplaceName: "desktop-tools" } }, - request, - }); - - expect(status.liveTest).toMatchObject({ - status: "failed", - ok: false, - attempts: 2, - retried: true, - repaired: false, - }); - expectStatusFields(status, { - ready: false, - reason: "live_test_failed", - installed: true, - pluginEnabled: true, - mcpServerAvailable: true, - }); - expect(status.warnings).toContain( - "Computer Use live test failed, but compatibility startup remains enabled; set computerUse.strictReadiness to true to fail closed.", - ); - expect(status.message).toContain( - "Startup is allowed because computerUse.strictReadiness is false.", - ); - expect(status.repair).toBeUndefined(); - expectRequestMethodNotCalled(request, "config/mcpServer/reload"); - }); - - it("surfaces install, exposure, and live-test layers separately when the live test fails", async () => { - const request = createComputerUseRequest({ installed: true, liveTestFailures: 2 }); - - const status = await readCodexComputerUseStatus({ - pluginConfig: { - computerUse: { - enabled: true, - marketplaceName: "desktop-tools", - autoRepair: true, - strictReadiness: true, - }, - }, - request, - }); - - expectStatusFields(status, { - ready: false, - reason: "live_test_failed", - installed: true, - pluginEnabled: true, - mcpServerAvailable: true, - }); - expect(status.installation).toMatchObject({ status: "installed", ok: true }); - expect(status.exposure).toMatchObject({ status: "available", ok: true }); - expect(status.liveTest).toMatchObject({ - status: "failed", - ok: false, - attempted: true, - attempts: 2, - timeoutMs: 60_000, - retried: true, - repaired: true, - error: "list_apps timed out", - }); - expect(status.message).toContain("Computer Use live test failed after 2 attempts"); - expect( - requestCalls(request).filter(([method]) => method === "config/mcpServer/reload"), - ).toHaveLength(1); - }); - - it.each([false, true])( - "skips live probes for non-strict startup (autoInstall: %s)", - async (autoInstall) => { - const request = createComputerUseRequest({ installed: !autoInstall, liveTestFailures: 2 }); - const status = await ensureCodexComputerUse({ - pluginConfig: { - computerUse: { enabled: true, autoInstall, marketplaceName: "desktop-tools" }, - }, - request, - }); - - expectStatusFields(status, { - ready: true, - reason: "ready", - installed: true, - pluginEnabled: true, - mcpServerAvailable: true, - }); - expect(status.liveTest).toMatchObject({ status: "skipped", ok: false, attempted: false }); - expectRequestMethodNotCalled(request, "thread/start"); - expectRequestMethodNotCalled(request, "mcpServer/tool/call"); - if (autoInstall) { - expect(request).toHaveBeenCalledWith("plugin/install", { - marketplacePath: "/marketplaces/desktop-tools/.agents/plugins/marketplace.json", - pluginName: "computer-use", - }); - } else { - expectRequestMethodNotCalled(request, "plugin/install"); - } - }, - ); - - it("fails startup closed when strictReadiness is enabled", async () => { - const request = createComputerUseRequest({ installed: true, liveTestFailures: 2 }); - - await expectSetupErrorStatus( - ensureCodexComputerUse({ - pluginConfig: { - computerUse: { - enabled: true, - marketplaceName: "desktop-tools", - strictReadiness: true, - }, - }, - request, - }), - { - ready: false, - reason: "live_test_failed", - installed: true, - pluginEnabled: true, - mcpServerAvailable: true, - }, - ); - }); - it("reports an installed but disabled Computer Use plugin separately", async () => { const request = createComputerUseRequest({ installed: true, enabled: false }); @@ -1849,196 +1402,6 @@ describe("Codex Computer Use setup", () => { }); }); -function createComputerUseRequest(params: { - installed: boolean; - enabled?: boolean; - pluginName?: string; - mcpServerName?: string; - mcpTools?: readonly string[]; - nativePluginsEnabled?: boolean | "absent"; - marketplaceAvailableAfterListCalls?: number; - liveTestFailures?: number; - liveTestResultErrors?: number; - reloadFailures?: number; - mcpToolsAvailable?: boolean; - remoteMarketplace?: { - name: string; - pluginId?: string | null; - }; - additionalMarketplaceNames?: readonly string[]; -}): CodexComputerUseRequest { - let installed = params.installed; - let enabled = params.enabled ?? installed; - let pluginListCalls = 0; - let liveTestFailures = params.liveTestFailures ?? 0; - let liveTestResultErrors = params.liveTestResultErrors ?? 0; - let reloadFailures = params.reloadFailures ?? 0; - let threadStartCalls = 0; - const pluginName = params.pluginName ?? "computer-use"; - const mcpServerName = params.mcpServerName ?? "computer-use"; - const mcpTools = params.mcpTools ?? ["list_apps"]; - const marketplaceName = params.remoteMarketplace?.name ?? "desktop-tools"; - const marketplacePath = params.remoteMarketplace - ? null - : `/marketplaces/${marketplaceName}/.agents/plugins/marketplace.json`; - const source = params.remoteMarketplace ? "remote" : "local"; - const currentPluginSummary = () => - pluginSummary( - installed, - marketplaceName, - enabled, - source, - params.remoteMarketplace?.pluginId, - pluginName, - ); - return vi.fn(async (method: string, requestParams?: unknown) => { - if (method === "experimentalFeature/enablement/set") { - return { - enablement: params.nativePluginsEnabled === false ? {} : { plugins: true }, - }; - } - if (method === "experimentalFeature/list") { - return { - data: - params.nativePluginsEnabled === "absent" - ? [] - : [{ name: "plugins", enabled: params.nativePluginsEnabled ?? true }], - nextCursor: null, - }; - } - if (method === "marketplace/add") { - return { - marketplaceName: "desktop-tools", - installedRoot: "/marketplaces/desktop-tools", - alreadyAdded: false, - }; - } - if (method === "plugin/list") { - pluginListCalls += 1; - const marketplaceAvailable = - pluginListCalls >= (params.marketplaceAvailableAfterListCalls ?? 1); - return { - marketplaces: marketplaceAvailable - ? [ - ...(params.additionalMarketplaceNames ?? []).map((name) => - marketplaceEntry(name, false), - ), - { - name: marketplaceName, - path: marketplacePath, - interface: null, - plugins: [currentPluginSummary()], - }, - ] - : [], - marketplaceLoadErrors: [], - featuredPluginIds: [], - }; - } - if (method === "plugin/read") { - expect(requestParams).toEqual( - params.remoteMarketplace - ? { - remoteMarketplaceName: marketplaceName, - pluginName: params.remoteMarketplace.pluginId, - } - : { marketplacePath, pluginName }, - ); - return { - plugin: { - marketplaceName, - marketplacePath, - summary: currentPluginSummary(), - description: "Control desktop apps.", - skills: [], - apps: [], - mcpServers: [mcpServerName], - }, - }; - } - if (method === "plugin/install") { - if (params.remoteMarketplace) { - expect(requestParams).toEqual({ - remoteMarketplaceName: marketplaceName, - pluginName: params.remoteMarketplace.pluginId, - }); - } - installed = true; - enabled = true; - return { authPolicy: "ON_INSTALL", appsNeedingAuth: [] }; - } - if (method === "config/mcpServer/reload") { - if (reloadFailures > 0) { - reloadFailures -= 1; - throw new Error("MCP runtime reload failed"); - } - return undefined; - } - if (method === "mcpServerStatus/list") { - return { - data: - installed && enabled - ? [ - { - name: mcpServerName, - tools: - params.mcpToolsAvailable === false - ? {} - : Object.fromEntries( - mcpTools.map((name) => [name, { name, inputSchema: { type: "object" } }]), - ), - resources: [], - resourceTemplates: [], - authStatus: "unsupported", - }, - ] - : [], - nextCursor: null, - }; - } - if (method === "thread/start") { - threadStartCalls += 1; - return { - thread: { - id: `computer-use-probe-thread-${threadStartCalls}`, - }, - model: "gpt-5.1", - modelProvider: "openai", - }; - } - if (method === "mcpServer/tool/call") { - const requestRecord = requireRecord(requestParams, "Computer Use readiness tool call"); - const tool = requestRecord.tool; - if (typeof tool !== "string" || !mcpTools.includes(tool)) { - return { - content: [{ type: "text", text: `Unknown tool: ${String(tool)}` }], - isError: true, - }; - } - expect(requestRecord).toEqual({ - threadId: `computer-use-probe-thread-${threadStartCalls}`, - server: mcpServerName, - tool, - arguments: tool === "js" ? { code: "await cua.getState();" } : {}, - }); - if (liveTestFailures > 0) { - liveTestFailures -= 1; - throw new Error(`${tool} timed out`); - } - if (liveTestResultErrors > 0) { - liveTestResultErrors -= 1; - return { content: [{ type: "text", text: `${tool} failed` }], isError: true }; - } - return { content: [{ type: "text", text: "[]" }] }; - } - if (method === "thread/unsubscribe" || method === "thread/archive") { - expect(requestParams).toEqual({ threadId: `computer-use-probe-thread-${threadStartCalls}` }); - return undefined; - } - throw new Error(`unexpected request ${method}`); - }) as CodexComputerUseRequest; -} - function createAmbiguousComputerUseRequest(): CodexComputerUseRequest { return vi.fn(async (method: string) => { if (method === "plugin/list") { @@ -2151,7 +1514,7 @@ function createMultiMarketplaceComputerUseRequest(): CodexComputerUseRequest { if (method === "mcpServer/tool/call") { return { content: [{ type: "text", text: "[]" }] }; } - if (method === "thread/unsubscribe" || method === "thread/archive") { + if (method === "thread/unsubscribe") { return undefined; } throw new Error(`unexpected request ${method}`); @@ -2295,7 +1658,7 @@ function createBundledMarketplaceComputerUseRequest( if (method === "mcpServer/tool/call") { return { content: [{ type: "text", text: "[]" }] }; } - if (method === "thread/unsubscribe" || method === "thread/archive") { + if (method === "thread/unsubscribe") { return undefined; } throw new Error(`unexpected request ${method}`); @@ -2324,36 +1687,4 @@ function createManagedMarketplaceHarness(root: string): { return { agentDir, client, managedMarketplacePath }; } -function marketplaceEntry(marketplaceName: string, installed: boolean) { - return { - name: marketplaceName, - path: `/marketplaces/${marketplaceName}/.agents/plugins/marketplace.json`, - interface: null, - plugins: [pluginSummary(installed, marketplaceName)], - }; -} - -function pluginSummary( - installed: boolean, - marketplaceName = "desktop-tools", - enabled = installed, - source: "local" | "remote" = "local", - remotePluginId?: string | null, - pluginName = "computer-use", -) { - return { - id: `${pluginName}@${marketplaceName}`, - ...(source === "remote" ? { remotePluginId: remotePluginId ?? null } : {}), - name: pluginName, - source: - source === "local" - ? { type: "local", path: `/marketplaces/${marketplaceName}/plugins/${pluginName}` } - : { type: "remote" }, - installed, - enabled, - installPolicy: "AVAILABLE", - authPolicy: "ON_INSTALL", - interface: null, - }; -} /* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */ diff --git a/extensions/codex/src/app-server/computer-use.ts b/extensions/codex/src/app-server/computer-use.ts index bbe12c06b79d..19555df7523b 100644 --- a/extensions/codex/src/app-server/computer-use.ts +++ b/extensions/codex/src/app-server/computer-use.ts @@ -15,6 +15,13 @@ import { type CodexAppServerClient, } from "./client.js"; import { resolveCodexManagedBundledMarketplacePath } from "./computer-use-marketplace.js"; +import { + runCodexComputerUseLiveTest, + skippedLiveTestStatus, + type CodexComputerUseLiveTestStatus, + type CodexComputerUseRepairStatus, + type CodexComputerUseRequest, +} from "./computer-use-readiness.js"; import { assertNotSymlink } from "./computer-use-service-path.js"; import { resolveCodexAppServerRuntimeOptions, @@ -28,7 +35,6 @@ import { } from "./desktop-app-paths.js"; import { isManagedCodexDesktopCommand } from "./managed-binary.js"; import { acquireCodexNativeConfigFence } from "./native-config-fence.js"; -import type { ToolCallResult as CodexMcpToolCallResult } from "./protocol-mcp.js"; import type { CodexAppServerRequestResult, CodexConfigReadResponse, @@ -38,28 +44,21 @@ import type { CodexPluginListResponse, CodexPluginReadResponse, CodexRequestObject, - CodexThreadStartResponse, JsonValue, } from "./protocol.js"; -import { requestCodexAppServerJson } from "./request.js"; +import { requestCodexAppServerClientJson } from "./request.js"; import { assertCodexAppServerClientStartSelectionCurrent, getLeasedSharedCodexAppServerClient, - isCodexAppServerStartSelectionChangedError, readCodexAppServerClientDesktopGeneration, readCodexAppServerClientProcessIdentity, releaseLeasedSharedCodexAppServerClient, resolveCodexNativeConfigFenceKey, waitForCodexAppServerClientDesktopGenerationDrain, + withLeasedCodexAppServerClientStartSelectionRetry, + type CodexAppServerClientLease, } from "./shared-client.js"; -/** Minimal app-server request function needed by Computer Use setup. */ -type CodexComputerUseRequest = ( - method: string, - params?: unknown, - options?: { timeoutMs?: number }, -) => Promise; - type CodexComputerUseStatusReason = | "disabled" | "marketplace_missing" @@ -80,34 +79,12 @@ type CodexComputerUseInstallationStatus = type CodexComputerUseExposureStatus = "skipped" | "missing" | "available"; -type CodexComputerUseLiveTestState = "skipped" | "passed" | "failed"; - -type CodexComputerUseRepairStatus = { - attempted: boolean; - killedPids: number[]; - message: string; - warnings: string[]; -}; - type CodexComputerUseStatusSection = { status: string; ok: boolean; message: string; }; -type CodexComputerUseLiveTestStatus = { - status: CodexComputerUseLiveTestState; - ok: boolean; - attempted: boolean; - attempts: number; - timeoutMs: number; - retried: boolean; - repaired: boolean; - message: string; - error?: string; - durationMs?: number; -}; - /** Readiness status for Codex Computer Use plugin and MCP server wiring. */ export type CodexComputerUseStatus = { enabled: boolean; @@ -146,7 +123,7 @@ class CodexComputerUseSetupError extends Error { /** Inputs for checking, ensuring, or installing Codex Computer Use support. */ export type CodexComputerUseSetupParams = { pluginConfig?: unknown; - config?: Parameters[0]["config"]; + config?: Parameters[0]["config"]; agentDir?: string; overrides?: Partial; /** Caller-owned injection seam for tests; production mutation safety requires `client`. */ @@ -222,12 +199,6 @@ const COMPUTER_USE_MARKETPLACE_NAME_PRIORITY = [ "openai-curated-remote", "local", ]; -const COMPUTER_USE_LIVE_TEST_RETRY_COUNT = 1; -const COMPUTER_USE_LIVE_TEST_THREAD_NAME = "OpenClaw Computer Use readiness probe"; -const COMPUTER_USE_LIST_APPS_TOOL = "list_apps"; -const COMPUTER_USE_UNIFIED_JS_TOOL = "js"; -const COMPUTER_USE_UNIFIED_JS_PROBE = "await cua.getState();"; - /** Reads Computer Use readiness without installing or mutating app-server state. */ export async function readCodexComputerUseStatus( params: CodexComputerUseSetupParams = {}, @@ -318,9 +289,6 @@ export async function installCodexComputerUse( async function inspectCodexComputerUse( params: CodexComputerUseInspectionParams, ): Promise { - if (!params.installPlugin) { - return await inspectCodexComputerUseWithoutFence(params); - } const resolvedRuntime = resolveCodexAppServerRuntimeOptions({ pluginConfig: params.pluginConfig, managedCommandOrder: "desktop-first", @@ -329,19 +297,66 @@ async function inspectCodexComputerUse( const deadline = operationTimeoutMs > 0 ? Date.now() + operationTimeoutMs : undefined; const remainingTimeoutMs = () => deadline === undefined ? operationTimeoutMs : Math.max(1, deadline - Date.now()); - let leasedClient: CodexAppServerClient | undefined; + const clientOptions = { + startOptions: resolvedRuntime.start, + pluginConfig: params.pluginConfig, + config: params.config, + agentDir: params.agentDir, + abandonSignal: params.signal, + }; + const lease: CodexAppServerClientLease = {}; try { let client = params.client; if (!client && !params.request) { client = await getLeasedSharedCodexAppServerClient({ - startOptions: resolvedRuntime.start, - pluginConfig: params.pluginConfig, + ...clientOptions, timeoutMs: remainingTimeoutMs(), - config: params.config, - agentDir: params.agentDir, - abandonSignal: params.signal, }); - leasedClient = client; + lease.client = client; + } + if (!params.installPlugin) { + if (!lease.client) { + return await inspectCodexComputerUseWithoutFence(params); + } + return await withLeasedCodexAppServerClientStartSelectionRetry({ + lease, + options: { ...clientOptions, timeoutMs: remainingTimeoutMs() }, + signal: params.signal, + run: async (readClient, requestOptions) => { + const { assertCurrent } = requestOptions(); + return await inspectCodexComputerUseWithoutFence({ + ...params, + client: readClient, + request: async ( + method: string, + requestParams?: unknown, + options?: { timeoutMs?: number; signal?: AbortSignal }, + ) => { + // Cleanup keeps its own deadline after the operation expires or is aborted. + const scopedOptions = + method === "thread/unsubscribe" + ? { + timeoutMs: options?.timeoutMs ?? operationTimeoutMs, + signal: options?.signal, + assertCurrent, + } + : requestOptions(); + return await requestCodexAppServerClientJson({ + client: readClient, + method, + requestParams, + config: params.config, + timeoutMs: Math.min( + options?.timeoutMs ?? operationTimeoutMs, + scopedOptions.timeoutMs, + ), + signal: scopedOptions.signal, + assertCurrent: scopedOptions.assertCurrent, + }); + }, + }); + }, + }); } const explicitManagedInstall = client && !resolveCodexComputerUseConfig({ pluginConfig: params.pluginConfig }).autoInstall @@ -403,8 +418,8 @@ async function inspectCodexComputerUse( } } } finally { - if (leasedClient) { - releaseLeasedSharedCodexAppServerClient(leasedClient); + if (lease.client) { + releaseLeasedSharedCodexAppServerClient(lease.client); } } } @@ -462,6 +477,8 @@ async function inspectCodexComputerUseWithoutFence( return await readComputerUseTools({ request, + client: params.client, + signal: params.signal, config: params.computerUseConfig, plugin: pluginInspection.plugin, runLiveTest: params.runLiveTest, @@ -606,6 +623,8 @@ async function ensureComputerUsePlugin(params: { async function readComputerUseTools(params: { request: CodexComputerUseRequest; + client?: CodexAppServerClient; + signal?: AbortSignal; config: ResolvedCodexComputerUseConfig; plugin: CodexPluginDetail; runLiveTest: boolean; @@ -654,6 +673,8 @@ async function readComputerUseTools(params: { params.releaseNativeConfigFence?.(); const { liveTest, repair } = await runCodexComputerUseLiveTest({ request: params.request, + client: params.client, + signal: params.signal, config: params.config, tools, }); @@ -681,139 +702,6 @@ async function readComputerUseTools(params: { }; } -export async function runCodexComputerUseLiveTest(params: { - request: CodexComputerUseRequest; - config: ResolvedCodexComputerUseConfig; - tools?: readonly string[]; -}): Promise<{ liveTest: CodexComputerUseLiveTestStatus; repair?: CodexComputerUseRepairStatus }> { - const startedAt = Date.now(); - let lastError: unknown; - let repair: CodexComputerUseRepairStatus | undefined; - const probe = resolveComputerUseLiveTestProbe(params.tools); - for (let attempt = 0; attempt <= COMPUTER_USE_LIVE_TEST_RETRY_COUNT; attempt += 1) { - let threadId: string | undefined; - try { - const thread = await params.request( - "thread/start", - { - input: [], - developerInstructions: COMPUTER_USE_LIVE_TEST_THREAD_NAME, - ephemeral: true, - }, - { - timeoutMs: params.config.liveTestTimeoutMs, - }, - ); - threadId = thread.thread.id; - const toolResult = await params.request( - "mcpServer/tool/call", - { - threadId, - server: params.config.mcpServerName, - tool: probe.tool, - arguments: probe.arguments, - }, - { - timeoutMs: params.config.toolCallTimeoutMs, - }, - ); - if (toolResult.isError === true) { - throw new Error( - `Computer Use readiness tool ${params.config.mcpServerName}.${probe.tool} returned an error result`, - ); - } - return { - liveTest: { - status: "passed", - ok: true, - attempted: true, - attempts: attempt + 1, - timeoutMs: params.config.liveTestTimeoutMs, - retried: attempt > 0, - repaired: Boolean(repair?.attempted && repair.warnings.length === 0), - durationMs: Math.max(0, Date.now() - startedAt), - message: "Computer Use live test passed.", - }, - ...(repair ? { repair } : {}), - }; - } catch (error) { - if (isCodexAppServerStartSelectionChangedError(error)) { - throw error; - } - lastError = error; - } finally { - if (threadId) { - await cleanupComputerUseProbeThread(params.request, threadId, params.config); - } - } - if (attempt < COMPUTER_USE_LIVE_TEST_RETRY_COUNT && params.config.autoRepair) { - repair = await repairComputerUseMcpRuntime(params.request, params.config); - } - } - const errorMessage = describeControlFailure(lastError); - return { - liveTest: { - status: "failed", - ok: false, - attempted: true, - attempts: COMPUTER_USE_LIVE_TEST_RETRY_COUNT + 1, - timeoutMs: params.config.liveTestTimeoutMs, - retried: COMPUTER_USE_LIVE_TEST_RETRY_COUNT > 0, - repaired: Boolean(repair?.attempted && repair.warnings.length === 0), - durationMs: Math.max(0, Date.now() - startedAt), - message: `Computer Use live test failed after ${COMPUTER_USE_LIVE_TEST_RETRY_COUNT + 1} attempts: ${errorMessage}`, - error: errorMessage, - }, - ...(repair ? { repair } : {}), - }; -} - -function resolveComputerUseLiveTestProbe(tools: readonly string[] | undefined): { - tool: string; - arguments: Record; -} { - if ( - tools?.includes(COMPUTER_USE_UNIFIED_JS_TOOL) && - !tools.includes(COMPUTER_USE_LIST_APPS_TOOL) - ) { - return { - tool: COMPUTER_USE_UNIFIED_JS_TOOL, - arguments: { code: COMPUTER_USE_UNIFIED_JS_PROBE }, - }; - } - return { tool: COMPUTER_USE_LIST_APPS_TOOL, arguments: {} }; -} - -async function repairComputerUseMcpRuntime( - request: CodexComputerUseRequest, - config: ResolvedCodexComputerUseConfig, -): Promise { - try { - // Codex owns MCP process lifetimes; signaling descendants can kill an active sibling. - await request("config/mcpServer/reload", undefined, { timeoutMs: config.liveTestTimeoutMs }); - return { - attempted: true, - killedPids: [], - warnings: [], - message: "Reloaded Computer Use MCP servers through Codex app-server.", - }; - } catch (error) { - const message = `Could not reload Computer Use MCP servers: ${describeControlFailure(error)}`; - return { attempted: true, killedPids: [], warnings: [message], message }; - } -} - -async function cleanupComputerUseProbeThread( - request: CodexComputerUseRequest, - threadId: string, - config: ResolvedCodexComputerUseConfig, -): Promise { - await Promise.allSettled([ - request("thread/unsubscribe", { threadId }, { timeoutMs: config.liveTestTimeoutMs }), - request("thread/archive", { threadId }, { timeoutMs: config.liveTestTimeoutMs }), - ]); -} - async function resolveMarketplaceRef(params: { request: CodexComputerUseRequest; config: ResolvedCodexComputerUseConfig; @@ -1290,22 +1178,6 @@ function exposureStatusFromTools( }; } -function skippedLiveTestStatus( - config: ResolvedCodexComputerUseConfig, - message: string, -): CodexComputerUseLiveTestStatus { - return { - status: "skipped", - ok: false, - attempted: false, - attempts: 0, - timeoutMs: config.liveTestTimeoutMs, - retried: false, - repaired: false, - message, - }; -} - function pluginWarnings(plugin: CodexPluginDetail): string[] { const warnings: string[] = []; const source = plugin.summary.source; @@ -1318,9 +1190,6 @@ function pluginWarnings(plugin: CodexPluginDetail): string[] { } function createComputerUseRequest(params: { - pluginConfig?: unknown; - config?: CodexComputerUseSetupParams["config"]; - agentDir?: string; request?: CodexComputerUseRequest; client?: CodexAppServerClient; timeoutMs?: number; @@ -1329,36 +1198,18 @@ function createComputerUseRequest(params: { if (params.request) { return params.request; } - if (params.client) { - return async ( - method: string, - requestParams?: unknown, - options?: { timeoutMs?: number }, - ) => - await params.client!.request(method, requestParams, { - timeoutMs: options?.timeoutMs ?? params.timeoutMs, - signal: params.signal, - }); + const client = params.client; + if (!client) { + throw new Error("Computer Use setup requires an acquired app-server client"); } - // One-off install/status overrides may enable Computer Use without persisting - // config first, so keep the desktop app entitlement owner for this client. - const runtime = resolveCodexAppServerRuntimeOptions({ - pluginConfig: params.pluginConfig, - managedCommandOrder: "desktop-first", - }); return async ( method: string, requestParams?: unknown, - options?: { timeoutMs?: number }, + options?: { timeoutMs?: number; signal?: AbortSignal }, ) => - await requestCodexAppServerJson({ - method, - requestParams, - timeoutMs: options?.timeoutMs ?? params.timeoutMs ?? runtime.requestTimeoutMs, - pluginConfig: params.pluginConfig, - startOptions: runtime.start, - config: params.config, - agentDir: params.agentDir, + await client.request(method, requestParams, { + timeoutMs: options?.timeoutMs ?? params.timeoutMs, + signal: options?.signal ?? params.signal, }); } diff --git a/extensions/codex/src/app-server/sandbox-exec-server.command-identity.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.command-identity.test.ts new file mode 100644 index 000000000000..de372d691d19 --- /dev/null +++ b/extensions/codex/src/app-server/sandbox-exec-server.command-identity.test.ts @@ -0,0 +1,67 @@ +import { useIsolatedStateGuard } from "openclaw/plugin-sdk/test-env"; +import { afterEach, describe, expect, it } from "vitest"; +import { sandboxExecServerRegistry } from "./sandbox-exec-server-registry.js"; +import { ensureCodexSandboxExecServerEnvironment } from "./sandbox-exec-server.js"; +import { + createClient, + createSandboxContext, + execServerUrlFromClient, + openSocket, + readUntilClosed, + rpc, +} from "./sandbox-exec-server.test-helpers.js"; + +useIsolatedStateGuard(); + +afterEach(async () => { + await sandboxExecServerRegistry.closeAll(); +}); + +describe("Codex sandbox command identity", () => { + it.runIf(process.platform !== "win32")( + "preserves literal argv process identity and exit status through a shell backend", + async () => { + const sandbox = createSandboxContext({ + buildExecSpec: async ({ command, env }) => ({ + argv: [ + "/bin/sh", + "-c", + // Prevent implicit shell exec optimization, which otherwise hides the extra process. + `export BACKEND_EXEC_PID=$$; trap 'printf "BACKEND_EXIT\\n"; exit 99' EXIT; ${command}`, + ], + env: { ...env, PATH: process.env.PATH }, + stdinMode: "pipe-closed", + }), + }); + const client = createClient(); + await ensureCodexSandboxExecServerEnvironment({ client: client as never, sandbox }); + const socket = await openSocket(execServerUrlFromClient(client)); + try { + await rpc(socket, "initialize", { clientName: "command-identity-test" }); + socket.send(JSON.stringify({ method: "initialized" })); + await rpc(socket, "process/start", { + processId: "literal-command", + argv: [ + process.execPath, + "-e", + "console.log(process.pid + ':' + process.env.BACKEND_EXEC_PID); process.exit(42)", + ], + cwd: "file:///workspace", + env: {}, + tty: false, + }); + const read = await readUntilClosed(socket, "literal-command"); + expect(read).toMatchObject({ exited: true, closed: true, exitCode: 42 }); + const output = (read.chunks ?? []) + .map(({ chunk }) => Buffer.from(chunk, "base64").toString("utf8")) + .join(""); + expect(output).toMatch(/^\d+:\d+\n$/u); + const [pid, backendPid] = output.trim().split(":").map(Number); + expect(pid).toBeGreaterThan(0); + expect(pid).toBe(backendPid); + } finally { + socket.close(); + } + }, + ); +}); diff --git a/extensions/codex/src/app-server/sandbox-exec-server.exit.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.exit.test.ts new file mode 100644 index 000000000000..2a4a1faa50df --- /dev/null +++ b/extensions/codex/src/app-server/sandbox-exec-server.exit.test.ts @@ -0,0 +1,234 @@ +import type { ChildProcess } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; +import { useIsolatedStateGuard } from "openclaw/plugin-sdk/test-env"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const spawnObservers = vi.hoisted(() => new Map void>()); +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: (...args: Parameters) => { + const child = actual.spawn(...args); + const script = Array.isArray(args[1]) ? args[1][0] : undefined; + if (script) { + spawnObservers.get(script)?.(child); + } + return child; + }, + }; +}); + +import type { JsonObject, JsonValue } from "./protocol.js"; +import { createSandboxContext } from "./sandbox-exec-server.test-helpers.js"; +import { requireNumber, requireObject, requireString } from "./sandbox-exec-server/json-rpc.js"; +import { CodexSandboxExecSession } from "./sandbox-exec-server/session.js"; +import type { OpenClawExecServer } from "./sandbox-exec-server/types.js"; +import { useAutoCleanupTempDirTracker } from "./test-support.js"; + +useIsolatedStateGuard(); +const tempDirs = useAutoCleanupTempDirTracker(afterEach); + +afterEach(() => { + spawnObservers.clear(); +}); + +describe("Codex sandbox process exit and output drain", () => { + it.runIf(process.platform !== "win32")( + "ignores interrupts after pipe exit while retaining late output and finalization", + async () => { + const directory = tempDirs.make("codex-process-exit-"); + const parentScript = path.join(directory, "parent.cjs"); + const descendantScript = path.join(directory, "descendant.cjs"); + const releaseFile = path.join(directory, "release"); + fs.writeFileSync( + parentScript, + [ + "const { spawn } = require('node:child_process');", + "const child = spawn(process.execPath, [process.argv[2], process.argv[3]], {", + " stdio: ['ignore', 'inherit', 'inherit', 'ipc'],", + "});", + "child.once('error', () => process.exit(8));", + "child.once('message', () => process.exit(7));", + ].join("\n"), + ); + fs.writeFileSync( + descendantScript, + [ + "const fs = require('node:fs');", + "const timer = setInterval(() => {", + " if (!fs.existsSync(process.argv[2])) return;", + " clearInterval(timer);", + " process.stdout.write('LATE_STDOUT\\n', () => {", + " process.stderr.write('LATE_STDERR\\n', () => process.exit(0));", + " });", + "}, 10);", + "process.send('ready');", + ].join("\n"), + ); + + const parentExited = createDeferred<{ code: number | null; signal: NodeJS.Signals | null }>(); + const parentClosed = createDeferred(); + void parentExited.promise.catch(() => undefined); + let parent: ChildProcess | undefined; + let closed = false; + spawnObservers.set(parentScript, (child) => { + parent = child; + child.once("exit", (code, signal) => parentExited.resolve({ code, signal })); + child.once("error", parentExited.reject); + child.once("close", () => { + closed = true; + parentClosed.resolve(); + }); + }); + + const finalizeExec = vi.fn(async () => undefined); + const runShellCommand = vi.fn(async () => ({ + code: 0, + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + })); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv: [process.execPath, parentScript, descendantScript, releaseFile], + env: { PATH: process.env.PATH }, + finalizeToken: "exit-drain-token", + stdinMode: "pipe-closed", + }), + finalizeExec, + runShellCommand, + }); + if (!sandbox.backend || !sandbox.fsBridge) { + throw new Error("The sandbox fixture must provide its backend and filesystem bridge"); + } + const execServer: OpenClawExecServer = { + environmentId: "exit-drain-test", + authPath: "/exit-drain-test", + refCount: 1, + closed: false, + url: "ws://localhost/exit-drain-test", + sandbox, + backend: sandbox.backend, + fsBridge: sandbox.fsBridge, + networkIsolated: true, + children: new Set(), + cleanupTasks: new Set(), + server: { clients: [], close: (callback) => callback() }, + }; + const messages: JsonObject[] = []; + const session = new CodexSandboxExecSession(execServer, { + send: (message) => messages.push(message), + isOpen: () => true, + }); + let nextRequestId = 0; + const request = async (method: string, params?: JsonValue) => { + const id = ++nextRequestId; + await session.handleRequest({ id, method, params }); + const response = messages.find((message) => message.id === id); + expect(response).toMatchObject({ jsonrpc: "2.0", id, result: expect.any(Object) }); + expect(response).not.toHaveProperty("error"); + return requireObject(response?.result, `${method} response`); + }; + let pendingRead: Promise | undefined; + try { + await request("initialize"); + await request("process/start", { + processId: "exit-drain", + argv: ["ignored"], + cwd: "file:///workspace", + tty: false, + }); + + // Observe the real OS child independently of the exec-server's projected state. + await vi.waitFor(() => expect(parent?.exitCode).toBe(7), { timeout: 5_000 }); + await expect(parentExited.promise).resolves.toEqual({ code: 7, signal: null }); + expect(closed).toBe(false); + const read = await request("process/read", { processId: "exit-drain", afterSeq: 0 }); + expect(read).toMatchObject({ exited: true, closed: false, exitCode: 7 }); + expect(finalizeExec).not.toHaveBeenCalled(); + await expect( + request("process/signal", { processId: "exit-drain", signal: "interrupt" }), + ).resolves.toEqual({}); + expect(runShellCommand).not.toHaveBeenCalled(); + + let readCompleted = false; + pendingRead = request("process/read", { + processId: "exit-drain", + afterSeq: requireNumber(read.nextSeq, "nextSeq") - 1, + waitMs: 1_000, + }).finally(() => { + readCompleted = true; + }); + void pendingRead.catch(() => undefined); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(readCompleted).toBe(false); + expect(closed).toBe(false); + expect(finalizeExec).not.toHaveBeenCalled(); + + fs.writeFileSync(releaseFile, "release"); + expect(await pendingRead).toMatchObject({ + exited: true, + exitCode: 7, + chunks: expect.arrayContaining([expect.objectContaining({ chunk: expect.any(String) })]), + }); + await vi.waitFor(() => expect(closed).toBe(true), { timeout: 5_000 }); + await parentClosed.promise; + await vi.waitFor(() => expect(finalizeExec).toHaveBeenCalledOnce()); + expect(finalizeExec).toHaveBeenCalledWith({ + status: "completed", + exitCode: 7, + timedOut: false, + token: "exit-drain-token", + }); + expect( + await request("process/read", { processId: "exit-drain", afterSeq: 0 }), + ).toMatchObject({ exited: true, closed: true, exitCode: 7 }); + + const notifications = messages.filter((message) => typeof message.method === "string"); + expect(notifications.filter((message) => message.method === "process/exited")).toHaveLength( + 1, + ); + expect(notifications.filter((message) => message.method === "process/closed")).toHaveLength( + 1, + ); + const output = notifications + .filter((message) => message.method === "process/output") + .map((message) => { + const params = requireObject(message.params, "process/output params"); + return Buffer.from(requireString(params.chunk, "chunk"), "base64").toString("utf8"); + }) + .join(""); + expect(output).toContain("LATE_STDOUT\n"); + expect(output).toContain("LATE_STDERR\n"); + expect(notifications.at(-1)?.method).toBe("process/closed"); + } finally { + fs.writeFileSync(releaseFile, "release"); + try { + if (parent) { + const forceCleanup = setTimeout(() => { + if (!closed && parent?.pid) { + try { + process.kill(-parent.pid, "SIGKILL"); + } catch (error) { + parentClosed.reject(error); + } + } + }, 5_000); + try { + await parentClosed.promise; + } finally { + clearTimeout(forceCleanup); + } + } + await pendingRead?.catch(() => undefined); + } finally { + await session.close(); + } + } + }, + ); +}); diff --git a/extensions/codex/src/app-server/sandbox-exec-server.http.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.http.test.ts index d0c04e80dc28..97b87eec71fd 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server.http.test.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server.http.test.ts @@ -2,7 +2,7 @@ import { spawn } from "node:child_process"; import { once } from "node:events"; import { writeFile } from "node:fs/promises"; -import { createServer, type IncomingMessage } from "node:http"; +import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { sandboxExecServerRegistry } from "./sandbox-exec-server-registry.js"; @@ -69,6 +69,7 @@ function splitUtf8ChildScript(params: { async function createLiveRedirectSandbox( targetHost: "source.test" | "target.test" | "127.0.0.1" | "private.test", redirectStatus = 302, + holdFinalResponse?: (response: ServerResponse) => void, ) { const requests: IncomingMessage[] = []; const requestBodies: string[] = []; @@ -93,12 +94,14 @@ async function createLiveRedirectSandbox( return; } response.writeHead(200, { "content-type": "text/plain" }); + if (holdFinalResponse) { + response.flushHeaders(); + holdFinalResponse(response); + return; + } response.end("final body"); }); }); - server.listen(0, "127.0.0.1"); - await once(server, "listening"); - const fixtureDir = tempDirs.make("codex-http-redirect-"); await writeFile( join(fixtureDir, "sitecustomize.py"), @@ -120,6 +123,8 @@ async function createLiveRedirectSandbox( "socket.socket.connect = connect", ].join("\n"), ); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); const env = { ...testExecEnv(), PYTHONPATH: fixtureDir }; const sandbox = createSandboxContext({ @@ -149,6 +154,7 @@ async function createLiveRedirectSandbox( requests, requestBodies, async close() { + server.closeAllConnections(); await new Promise((resolve, reject) => { server.close((error) => (error ? reject(error) : resolve())); }); @@ -157,19 +163,62 @@ async function createLiveRedirectSandbox( } describe("OpenClaw Codex sandbox exec-server HTTP", () => { + it("cancels an outstanding nonstreaming HTTP response when its exec-server socket closes", async () => { + const responseClosed = vi.fn(); + let heldResponse: ServerResponse | undefined; + const fixture = await createLiveRedirectSandbox("source.test", 302, (response) => { + heldResponse = response; + response.once("close", responseClosed); + }); + try { + const socket = await openSandboxHttpSocket(fixture.sandbox); + try { + await rpc(socket, "initialize", { clientName: "test" }); + socket.send( + JSON.stringify({ + id: 2, + method: "http/request", + params: { requestId: "pending-http", method: "GET", url: fixture.url }, + }), + ); + await vi.waitFor(() => expect(heldResponse).toBeDefined()); + + socket.terminate(); + + await vi.waitFor(() => expect(responseClosed).toHaveBeenCalledOnce(), { + timeout: 5_000, + }); + } finally { + socket.terminate(); + } + } finally { + heldResponse?.destroy(); + await fixture.close(); + } + }); + it("routes HTTP requests through the sandbox backend", async () => { - const runShellCommand = vi.fn(async () => ({ - stdout: Buffer.from( - JSON.stringify({ - status: 201, - headers: [{ name: "content-type", value: "text/plain" }], - bodyBase64: Buffer.from("sandbox-http").toString("base64"), - }), - ), - stderr: Buffer.alloc(0), - code: 0, - })); - const sandbox = createSandboxContext({ runShellCommand }); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv: [ + process.execPath, + "-e", + [ + "let input = '';", + "process.stdin.setEncoding('utf8');", + "process.stdin.on('data', chunk => input += chunk);", + "process.stdin.on('end', () => {", + " const request = JSON.parse(input);", + " process.stdout.write(JSON.stringify({", + " status: 201, headers: request.headers, bodyBase64: request.bodyBase64,", + " }));", + "});", + ].join("\n"), + ], + env: testExecEnv(), + stdinMode: "pipe-closed", + }), + }); const socket = await openSandboxHttpSocket(sandbox); await rpc(socket, "initialize", { clientName: "test" }); socket.send(JSON.stringify({ method: "initialized" })); @@ -184,25 +233,19 @@ describe("OpenClaw Codex sandbox exec-server HTTP", () => { }), ).resolves.toEqual({ status: 201, - headers: [{ name: "content-type", value: "text/plain" }], - bodyBase64: Buffer.from("sandbox-http").toString("base64"), + headers: [{ name: "authorization", value: "Bearer test" }], + bodyBase64: Buffer.from("body").toString("base64"), }); - expect(runShellCommand).toHaveBeenCalledWith( - expect.objectContaining({ - allowFailure: true, - stdin: expect.stringContaining("https://example.test/mcp"), - }), - ); socket.close(); }); it("blocks private HTTP targets before starting the sandbox backend", async () => { - const runShellCommand = vi.fn(async () => ({ - stdout: Buffer.alloc(0), - stderr: Buffer.alloc(0), - code: 0, + const buildExecSpec = vi.fn(async () => ({ + argv: [process.execPath, "-e", ""], + env: testExecEnv(), + stdinMode: "pipe-closed" as const, })); - const sandbox = createSandboxContext({ runShellCommand }); + const sandbox = createSandboxContext({ buildExecSpec }); const socket = await openSandboxHttpSocket(sandbox); await rpc(socket, "initialize", { clientName: "test" }); socket.send(JSON.stringify({ method: "initialized" })); @@ -214,7 +257,7 @@ describe("OpenClaw Codex sandbox exec-server HTTP", () => { url: "http://127.0.0.1:6379/", }), ).rejects.toThrow("Blocked hostname or private/internal IP"); - expect(runShellCommand).not.toHaveBeenCalled(); + expect(buildExecSpec).not.toHaveBeenCalled(); socket.close(); }); diff --git a/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts index 108b817c4370..da3f769b0146 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server.lifecycle.test.ts @@ -1,28 +1,36 @@ // Codex tests cover sandbox exec-server child and backend lease lifecycle ordering. import type { ChildProcessWithoutNullStreams } from "node:child_process"; -import { EventEmitter } from "node:events"; +import { EventEmitter, once } from "node:events"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { PassThrough } from "node:stream"; -import type { SandboxContext } from "openclaw/plugin-sdk/sandbox"; +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; +import { SANDBOX_COMMAND_MAX_BUFFER_BYTES, type SandboxContext } from "openclaw/plugin-sdk/sandbox"; import { useIsolatedStateGuard, withEnvAsync } from "openclaw/plugin-sdk/test-env"; import { afterEach, describe, expect, it, vi } from "vitest"; const spawnMock = vi.hoisted(() => vi.fn()); -const killProcessTreeMock = vi.hoisted(() => vi.fn()); +const signalProcessTreeMock = vi.hoisted(() => vi.fn()); vi.mock("node:child_process", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - spawn: (...args: Parameters) => spawnMock(...args), + spawn: (...args: Parameters) => { + const child = spawnMock(...args); + void Promise.resolve().then(() => child.emit("spawn")); + return child; + }, }; }); vi.mock("openclaw/plugin-sdk/process-runtime", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - killProcessTree: (...args: unknown[]) => killProcessTreeMock(...args), + signalProcessTree: (...args: Parameters) => { + signalProcessTreeMock(...args); + args[2]?.onComplete?.(); + }, }; }); @@ -35,6 +43,9 @@ import type { ManagedProcess, OpenClawExecServer, } from "./sandbox-exec-server/types.js"; +import { useAutoCleanupTempDirTracker } from "./test-support.js"; + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); type FakeNotifications = CodexSandboxExecSessionNotifications & { send: ReturnType>; @@ -92,15 +103,279 @@ function streamingHttpParams(requestId: string) { }; } +async function createPendingRemoteSignalFixture(holdFirstScan = false) { + const { spawn: spawnReal } = + await vi.importActual("node:child_process"); + const transport = createFakeChild(); + spawnMock.mockReturnValue(transport); + signalProcessTreeMock.mockImplementation(() => transport.emit("close", 143, "SIGTERM")); + const procRoot = tempDirs.make("codex-interrupt-procfs-"); + const firstScanEntered = createDeferred(); + const firstScanCompleted = createDeferred(); + const releaseFirstScan = createDeferred(); + const finalizeExec = vi.fn(async () => undefined); + const send = vi.fn(); + let marker = ""; + let firstScan = true; + let receiver: ChildProcessWithoutNullStreams | undefined; + let receiverClosed: Promise | undefined; + let receivedSignal = false; + let transportClosed = false; + transport.once("close", () => { + transportClosed = true; + }); + const sandbox = createSandboxContext({ + buildExecSpec: async ({ env }) => { + marker = env.CODEX_SANDBOX_EXEC_ID ?? ""; + expect(marker).not.toBe(""); + return { argv: ["pending-remote-transport"], env: {}, stdinMode: "pipe-closed" }; + }, + finalizeExec, + runShellCommand: async ({ script, args, signal }) => { + const initial = firstScan; + firstScan = false; + if (initial) { + firstScanEntered.resolve(); + if (holdFirstScan) { + await releaseFirstScan.promise; + } + } + // Run the real helper against a controlled Linux procfs view on either POSIX host. + const helper = spawnReal( + "/bin/sh", + ["-c", script.replaceAll("/proc/", `${procRoot}/`), "remote-signal-test", ...(args ?? [])], + { env: { PATH: process.env.PATH }, signal }, + ); + helper.stdin.end(); + const stdout: Buffer[] = []; + const stderr: Buffer[] = []; + helper.stdout.on("data", (chunk: Buffer) => stdout.push(chunk)); + helper.stderr.on("data", (chunk: Buffer) => stderr.push(chunk)); + const [code] = await once(helper, "close"); + if (initial) { + firstScanCompleted.resolve(); + } + return { + code: typeof code === "number" ? code : 1, + stdout: Buffer.concat(stdout), + stderr: Buffer.concat(stderr), + }; + }, + }); + const session = new CodexSandboxExecSession(createExecServer(sandbox), { + send, + isOpen: () => true, + }); + return { + session, + send, + finalizeExec, + firstScanEntered: firstScanEntered.promise, + firstScanCompleted: firstScanCompleted.promise, + releaseFirstScan: releaseFirstScan.resolve, + get receivedSignal() { + return receivedSignal; + }, + get transportClosed() { + return transportClosed; + }, + async admitReceiver() { + const ready = createDeferred(); + let output = ""; + receiver = spawnReal( + process.execPath, + [ + "-e", + "process.on('SIGINT', () => process.stdout.write('REMOTE_INT\\n', () => process.exit(42))); process.stdout.write('READY\\n'); setInterval(() => {}, 1000);", + ], + { env: { PATH: process.env.PATH, CODEX_SANDBOX_EXEC_ID: marker } }, + ); + receiver.stdout.on("data", (chunk: Buffer) => { + output += chunk.toString("utf8"); + if (output.includes("READY\n")) { + ready.resolve(); + } + receivedSignal ||= output.includes("REMOTE_INT\n"); + transport.stdout.emit("data", chunk); + }); + receiver.stderr.on("data", (chunk: Buffer) => transport.stderr.emit("data", chunk)); + const procEntry = path.join(procRoot, String(receiver.pid)); + receiverClosed = once(receiver, "close").then(([code, signal]) => { + fs.rmSync(procEntry, { recursive: true, force: true }); + transport.emit("close", code, signal); + }); + await Promise.race([ + ready.promise, + receiverClosed.then(() => { + throw new Error("Remote receiver exited before admission"); + }), + ]); + fs.mkdirSync(procEntry); + fs.writeFileSync(path.join(procEntry, "environ"), `CODEX_SANDBOX_EXEC_ID=${marker}\0`); + }, + async cleanup() { + releaseFirstScan.resolve(); + receiver?.kill("SIGKILL"); + await receiverClosed; + if (!transportClosed) { + transport.emit("close", 143, "SIGTERM"); + } + await session.close(); + }, + }; +} + useIsolatedStateGuard(); afterEach(() => { vi.useRealTimers(); spawnMock.mockReset(); - killProcessTreeMock.mockReset(); + signalProcessTreeMock.mockReset(); }); describe("Codex sandbox exec-server lifecycle", () => { + it("bounds interruption of a never-admitted remote process and settles cleanup", async () => { + vi.useFakeTimers(); + const child = createFakeChild(); + spawnMock.mockReturnValue(child); + signalProcessTreeMock.mockImplementation(() => child.emit("close", 143, "SIGTERM")); + let interrupting = true; + const finalizeExec = vi.fn(async () => undefined); + const send = vi.fn(); + const session = new CodexSandboxExecSession( + createExecServer( + createSandboxContext({ + finalizeExec, + runShellCommand: async () => ({ + code: interrupting ? 75 : 0, + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + }), + }), + ), + { send, isOpen: () => true }, + ); + let interrupt: Promise | undefined; + try { + await session.handleRequest({ + id: 1, + method: "process/start", + params: processStartParams("never-admitted"), + }); + let completed = false; + interrupt = session + .handleRequest({ + id: 2, + method: "process/signal", + params: { processId: "never-admitted", signal: "interrupt" }, + }) + .then(() => { + completed = true; + }); + await vi.advanceTimersByTimeAsync(4_499); + expect(completed).toBe(false); + expect(finalizeExec).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1); + await interrupt; + expect(send).toHaveBeenCalledWith({ + jsonrpc: "2.0", + id: 2, + error: { code: -32603, message: expect.stringMatching(/interrupt/iu) }, + }); + interrupting = false; + await session.close(); + expect(finalizeExec).toHaveBeenCalledOnce(); + } finally { + interrupting = false; + child.emit("close", 143, "SIGTERM"); + await vi.advanceTimersByTimeAsync(4_500); + await Promise.all([interrupt, session.close()]); + } + }); + + it.skipIf(process.platform === "win32")( + "waits for remote admission before acknowledging a process interrupt", + async () => { + const fixture = await createPendingRemoteSignalFixture(); + let interrupt: Promise | undefined; + try { + await fixture.session.handleRequest({ + id: 1, + method: "process/start", + params: processStartParams("late-remote-process"), + }); + interrupt = fixture.session.handleRequest({ + id: 2, + method: "process/signal", + params: { processId: "late-remote-process", signal: "interrupt" }, + }); + await fixture.firstScanCompleted; + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(fixture.send.mock.calls.some(([message]) => message.id === 2)).toBe(false); + expect(fixture.receivedSignal).toBe(false); + + await fixture.admitReceiver(); + await interrupt; + await vi.waitFor(() => expect(fixture.receivedSignal).toBe(true), { timeout: 5_000 }); + await vi.waitFor(() => + expect(fixture.send).toHaveBeenCalledWith({ + jsonrpc: "2.0", + method: "process/exited", + params: expect.objectContaining({ processId: "late-remote-process", exitCode: 42 }), + }), + ); + expect(fixture.send).toHaveBeenCalledWith({ jsonrpc: "2.0", id: 2, result: {} }); + } finally { + await fixture.cleanup(); + await interrupt; + } + }, + ); + + it.skipIf(process.platform === "win32")( + "joins a pending remote interrupt before finalizing a closed session", + async () => { + const fixture = await createPendingRemoteSignalFixture(true); + let interrupt: Promise | undefined; + let cleanup: Promise | undefined; + try { + await fixture.session.handleRequest({ + id: 1, + method: "process/start", + params: processStartParams("closing-remote-process"), + }); + interrupt = fixture.session.handleRequest({ + id: 2, + method: "process/signal", + params: { processId: "closing-remote-process", signal: "interrupt" }, + }); + await fixture.firstScanEntered; + let closed = false; + cleanup = fixture.session.close().then(() => { + closed = true; + }); + await vi.waitFor(() => expect(fixture.transportClosed).toBe(true), { timeout: 5_000 }); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(fixture.finalizeExec).not.toHaveBeenCalled(); + expect(closed).toBe(false); + + fixture.releaseFirstScan(); + await Promise.all([interrupt, cleanup]); + expect(fixture.receivedSignal).toBe(false); + expect(fixture.finalizeExec).toHaveBeenCalledOnce(); + } finally { + fixture.releaseFirstScan(); + await fixture.cleanup(); + await Promise.all([interrupt, cleanup]); + } + }, + ); + it.each([ { key: "HOME", via: "path" }, { key: "OPENCLAW_STATE_DIR", via: "path" }, @@ -147,6 +422,11 @@ describe("Codex sandbox exec-server lifecycle", () => { const child = createFakeChild(); spawnMock.mockReturnValue(child); const finalizeExec = vi.fn(async () => undefined); + const runShellCommand = vi.fn(async () => ({ + code: 0, + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + })); const sandbox = createSandboxContext({ buildExecSpec: async () => ({ argv: ["sandbox-child"], @@ -155,6 +435,7 @@ describe("Codex sandbox exec-server lifecycle", () => { stdinMode: "pipe-closed", }), finalizeExec, + runShellCommand, }); const send = vi.fn(); const session = new CodexSandboxExecSession(createExecServer(sandbox), { @@ -199,7 +480,7 @@ describe("Codex sandbox exec-server lifecycle", () => { { jsonrpc: "2.0", method: "process/exited", - params: { processId: "direct-session", seq: 2, exitCode: 0 }, + params: { processId: "direct-session", seq: 2, exitCode: 0, sandboxDenied: false }, }, { jsonrpc: "2.0", @@ -211,6 +492,7 @@ describe("Codex sandbox exec-server lifecycle", () => { expect(session.close()).toBe(cleanup); await cleanup; expect(finalizeExec).toHaveBeenCalledOnce(); + expect(runShellCommand).not.toHaveBeenCalled(); }); it("reaps and finalizes a TERM-resistant child before acknowledging termination", async () => { @@ -240,7 +522,7 @@ describe("Codex sandbox exec-server lifecycle", () => { createFakeNotifications().send, processStartParams("process-resistant"), ); - killProcessTreeMock.mockImplementation(() => { + signalProcessTreeMock.mockImplementation(() => { setTimeout(() => child.emit("close", null, "SIGKILL"), 1_000); }); @@ -254,9 +536,9 @@ describe("Codex sandbox exec-server lifecycle", () => { await vi.advanceTimersByTimeAsync(0); expect(settled).toBe(false); - expect(killProcessTreeMock).toHaveBeenCalledWith(child.pid, { + expect(signalProcessTreeMock).toHaveBeenCalledWith(child.pid, "SIGTERM", { detached: process.platform !== "win32", - graceMs: 1_000, + onComplete: expect.any(Function), }); await vi.runOnlyPendingTimersAsync(); @@ -276,7 +558,7 @@ describe("Codex sandbox exec-server lifecycle", () => { it("preserves cooperative TERM exit without force killing", async () => { const child = createFakeChild(); spawnMock.mockReturnValue(child); - killProcessTreeMock.mockImplementation(() => child.emit("close", 143, "SIGTERM")); + signalProcessTreeMock.mockImplementation(() => child.emit("close", 143, "SIGTERM")); const finalizeExec = vi.fn(async () => undefined); const processes = new Map(); await startProcess( @@ -300,7 +582,7 @@ describe("Codex sandbox exec-server lifecycle", () => { terminateProcess(processes, { processId: "process-cooperative" }), ).resolves.toEqual({ running: true }); - expect(killProcessTreeMock).toHaveBeenCalledOnce(); + expect(signalProcessTreeMock).toHaveBeenCalledOnce(); expect(finalizeExec).toHaveBeenCalledWith({ status: "completed", exitCode: 143, @@ -313,7 +595,7 @@ describe("Codex sandbox exec-server lifecycle", () => { vi.useFakeTimers(); const child = createFakeChild(); spawnMock.mockReturnValue(child); - killProcessTreeMock.mockImplementation(() => { + signalProcessTreeMock.mockImplementation(() => { setTimeout(() => child.emit("close", null, "SIGKILL"), 1_000); }); const finalizeExec = vi.fn(async () => undefined); @@ -343,7 +625,7 @@ describe("Codex sandbox exec-server lifecycle", () => { { running: true }, { running: true }, ]); - expect(killProcessTreeMock).toHaveBeenCalledOnce(); + expect(signalProcessTreeMock).toHaveBeenCalledOnce(); expect(finalizeExec).toHaveBeenCalledOnce(); }); @@ -351,7 +633,7 @@ describe("Codex sandbox exec-server lifecycle", () => { vi.useFakeTimers(); const child = createFakeChild(); spawnMock.mockReturnValue(child); - killProcessTreeMock.mockImplementation(() => undefined); + signalProcessTreeMock.mockImplementation(() => undefined); const finalizeExec = vi.fn(async () => undefined); const processes = new Map(); await startProcess( @@ -385,7 +667,7 @@ describe("Codex sandbox exec-server lifecycle", () => { vi.useFakeTimers(); const child = createFakeChild(); spawnMock.mockReturnValue(child); - killProcessTreeMock.mockImplementation(() => { + signalProcessTreeMock.mockImplementation(() => { setTimeout(() => child.emit("close", null, "SIGKILL"), 1_000); }); const finalizeExec = vi.fn(async () => undefined); @@ -404,6 +686,7 @@ describe("Codex sandbox exec-server lifecycle", () => { ), notifications, streamingHttpParams("http-resistant"), + new Set(), ); await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledOnce()); (child.stdout as PassThrough).write( @@ -414,7 +697,7 @@ describe("Codex sandbox exec-server lifecycle", () => { notifications.close(); await vi.runOnlyPendingTimersAsync(); - expect(killProcessTreeMock).toHaveBeenCalledOnce(); + expect(signalProcessTreeMock).toHaveBeenCalledOnce(); expect(finalizeExec).toHaveBeenCalledOnce(); expect(finalizeExec).toHaveBeenCalledWith({ status: "failed", @@ -424,6 +707,165 @@ describe("Codex sandbox exec-server lifecycle", () => { }); }); + it.each([true, false])( + "joins pending HTTP preparation without launching after close (stream=%s)", + async (streamResponse) => { + let preparing = false; + const releasePreparation = createDeferred(); + const child = createFakeChild(); + spawnMock.mockImplementation(() => { + setImmediate(() => child.emit("close", 0, null)); + return child; + }); + const finalizeExec = vi.fn(async () => undefined); + const session = new CodexSandboxExecSession( + createExecServer( + createSandboxContext({ + buildExecSpec: async () => { + preparing = true; + await releasePreparation.promise; + return { + argv: ["sandbox-http-child"], + env: {}, + finalizeToken: "cancelled-http-preparation", + stdinMode: "pipe-closed", + }; + }, + finalizeExec, + }), + ), + { send: vi.fn(), isOpen: () => true }, + ); + const request = session.handleRequest({ + id: 1, + method: "http/request", + params: { ...streamingHttpParams("preparing-http"), streamResponse }, + }); + try { + await vi.waitFor(() => expect(preparing).toBe(true)); + let closed = false; + const cleanup = session.close().then(() => { + closed = true; + }); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(closed).toBe(false); + releasePreparation.resolve(); + await Promise.all([request, cleanup]); + + expect(spawnMock).not.toHaveBeenCalled(); + expect(finalizeExec).toHaveBeenCalledExactlyOnceWith({ + status: "failed", + exitCode: null, + timedOut: false, + token: "cancelled-http-preparation", + }); + } finally { + releasePreparation.resolve(); + await Promise.all([request, session.close()]); + } + }, + ); + + it("joins streaming HTTP finalization after returning headers", async () => { + let finalizing = false; + const releaseFinalization = createDeferred(); + const child = createFakeChild(); + spawnMock.mockReturnValue(child); + signalProcessTreeMock.mockImplementation(() => child.emit("close", 143, "SIGTERM")); + const session = new CodexSandboxExecSession( + createExecServer( + createSandboxContext({ + finalizeExec: async () => { + finalizing = true; + await releaseFinalization.promise; + }, + }), + ), + { send: vi.fn(), isOpen: () => true }, + ); + const request = session.handleRequest({ + id: 1, + method: "http/request", + params: streamingHttpParams("finalizing-http"), + }); + try { + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledOnce()); + (child.stdout as PassThrough).write( + `${JSON.stringify({ type: "headers", status: 200, headers: [] })}\n`, + ); + await request; + let closed = false; + const cleanup = session.close().then(() => { + closed = true; + }); + await vi.waitFor(() => expect(finalizing).toBe(true)); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(closed).toBe(false); + releaseFinalization.resolve(); + await cleanup; + } finally { + releaseFinalization.resolve(); + await Promise.all([request, session.close()]); + } + }); + + it.each(["stdout", "stderr"] as const)( + "preserves the nonstreaming HTTP byte limit for %s and settles overflow cleanup", + async (stream) => { + const child = createFakeChild(); + spawnMock.mockReturnValue(child); + signalProcessTreeMock.mockImplementation(() => child.emit("close", 143, "SIGTERM")); + const finalizeExec = vi.fn(async () => undefined); + const operations = new Set>(); + const request = httpRequest( + createExecServer(createSandboxContext({ finalizeExec })), + createFakeNotifications(), + { ...streamingHttpParams("http-buffer-limit"), streamResponse: false }, + operations, + ); + const response = request.catch((error: unknown) => error); + try { + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledOnce()); + const output = child[stream] as PassThrough; + // Reuse backing memory while exercising the real per-stream byte threshold. + const chunk = Buffer.alloc(1024 * 1024, "x"); + for ( + let remaining = SANDBOX_COMMAND_MAX_BUFFER_BYTES; + remaining > 0; + remaining -= chunk.length + ) { + output.write(chunk.subarray(0, Math.min(remaining, chunk.length))); + } + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(signalProcessTreeMock).not.toHaveBeenCalled(); + + output.write(Buffer.from("x")); + + await vi.waitFor(() => expect(signalProcessTreeMock).toHaveBeenCalledOnce()); + expect(await response).toMatchObject({ + message: `sandbox http/request ${stream} exceeded ${SANDBOX_COMMAND_MAX_BUFFER_BYTES} bytes`, + }); + await Promise.all(operations); + expect(finalizeExec).toHaveBeenCalledExactlyOnceWith({ + status: "failed", + exitCode: 143, + timedOut: false, + token: undefined, + }); + } finally { + child.emit("close", 143, "SIGTERM"); + await response; + await Promise.allSettled(operations); + } + }, + ); + it("retains the process backend lease after child error until close", async () => { const child = createFakeChild(); spawnMock.mockReturnValue(child); @@ -514,9 +956,11 @@ describe("Codex sandbox exec-server lifecycle", () => { }); }); - it("retains the streaming HTTP backend lease after child error until close", async () => { + it("retains the streaming HTTP backend lease through close and remote cleanup after child error", async () => { const child = createFakeChild(); spawnMock.mockReturnValue(child); + const releaseRemoteCleanup = createDeferred(); + let remoteCleanupStarted = false; const finalizeExec = vi.fn(async () => undefined); const sandbox = createSandboxContext({ buildExecSpec: async () => ({ @@ -526,11 +970,18 @@ describe("Codex sandbox exec-server lifecycle", () => { stdinMode: "pipe-closed", }), finalizeExec, + runShellCommand: async () => { + remoteCleanupStarted = true; + await releaseRemoteCleanup.promise; + return { code: 0, stdout: Buffer.alloc(0), stderr: Buffer.alloc(0) }; + }, }); + const operations = new Set>(); const request = httpRequest( createExecServer(sandbox), createFakeNotifications(), streamingHttpParams("http-error"), + operations, ); let settled = false; void request.then( @@ -550,16 +1001,25 @@ describe("Codex sandbox exec-server lifecycle", () => { expect(settled).toBe(false); expect(finalizeExec).not.toHaveBeenCalled(); - const rejection = expect(request).rejects.toThrow("HTTP child transport failed"); - child.emit("close", 29, null); - await rejection; - await vi.waitFor(() => expect(finalizeExec).toHaveBeenCalledOnce()); - expect(finalizeExec).toHaveBeenCalledWith({ - status: "failed", - exitCode: 29, - timedOut: false, - token: "http-token", - }); + try { + const rejection = expect(request).rejects.toThrow("HTTP child transport failed"); + child.emit("close", 29, null); + await rejection; + await vi.waitFor(() => expect(remoteCleanupStarted).toBe(true)); + expect(finalizeExec).not.toHaveBeenCalled(); + + releaseRemoteCleanup.resolve(); + await Promise.all(operations); + expect(finalizeExec).toHaveBeenCalledExactlyOnceWith({ + status: "failed", + exitCode: 29, + timedOut: false, + token: "http-token", + }); + } finally { + releaseRemoteCleanup.resolve(); + await Promise.all(operations); + } }); it.each([ @@ -591,6 +1051,7 @@ describe("Codex sandbox exec-server lifecycle", () => { createExecServer(sandbox), createFakeNotifications(), streamingHttpParams("http-start-failure"), + new Set(), ), ).rejects.toThrow(spawnError ?? "did not provide a command"); expect(finalizeExec).toHaveBeenCalledOnce(); diff --git a/extensions/codex/src/app-server/sandbox-exec-server.spawn-error.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.spawn-error.test.ts new file mode 100644 index 000000000000..d38469ca57b9 --- /dev/null +++ b/extensions/codex/src/app-server/sandbox-exec-server.spawn-error.test.ts @@ -0,0 +1,144 @@ +import { useIsolatedStateGuard } from "openclaw/plugin-sdk/test-env"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const spawnFault = vi.hoisted(() => { + const state: { + code: "EMFILE" | "ENFILE"; + errors: Error[]; + close?: () => void; + } = { code: "EMFILE", errors: [] }; + return state; +}); + +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + const { EventEmitter } = await import("node:events"); + const { constants } = await import("node:os"); + return { + ...actual, + spawn: (...args: Parameters) => { + if (args[0] !== "codex-spawn-resource-fault") { + return actual.spawn(...args); + } + // Node 24/26 returns before assigning any stdio fields on EMFILE/ENFILE. + const child = new EventEmitter(); + const error = Object.assign(new Error(`spawn ${args[0]} ${spawnFault.code}`), { + code: spawnFault.code, + }); + child.once("error", (emitted: Error) => spawnFault.errors.push(emitted)); + spawnFault.close = () => child.emit("close", -constants.errno[spawnFault.code], null); + process.nextTick(() => child.emit("error", error)); + return child; + }, + }; +}); + +import { createSandboxContext } from "./sandbox-exec-server.test-helpers.js"; +import { CodexSandboxExecSession } from "./sandbox-exec-server/session.js"; +import type { JsonRpcRequest, OpenClawExecServer } from "./sandbox-exec-server/types.js"; + +useIsolatedStateGuard(); + +afterEach(() => { + spawnFault.errors = []; + spawnFault.close = undefined; +}); + +const requests: Array<{ label: string; request: JsonRpcRequest }> = [ + { + label: "process/start", + request: { + id: 1, + method: "process/start", + params: { + processId: "spawn-failure", + argv: ["true"], + cwd: "file:///workspace", + tty: false, + pipeStdin: false, + }, + }, + }, + ...[false, true].map((streamResponse) => ({ + label: `http/request streaming=${streamResponse}`, + request: { + id: 1, + method: "http/request", + params: { + requestId: "spawn-failure", + method: "GET", + url: "https://example.test/response", + streamResponse, + }, + }, + })), +]; + +describe.each(["EMFILE", "ENFILE"] as const)("sandbox spawn %s", (code) => { + it.each(requests)("settles $label after the failed child closes", async ({ request }) => { + spawnFault.code = code; + const finalizeExec = vi.fn(async () => undefined); + const runShellCommand = vi.fn(async () => ({ + code: 0, + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + })); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv: ["codex-spawn-resource-fault"], + env: {}, + finalizeToken: "spawn-failure-token", + stdinMode: "pipe-closed", + }), + finalizeExec, + runShellCommand, + }); + if (!sandbox.backend || !sandbox.fsBridge) { + throw new Error("The sandbox fixture must provide its backend and filesystem bridge"); + } + const execServer: OpenClawExecServer = { + environmentId: "spawn-failure-test", + authPath: "/spawn-failure-test", + refCount: 1, + closed: false, + url: "ws://localhost/spawn-failure-test", + sandbox, + backend: sandbox.backend, + fsBridge: sandbox.fsBridge, + networkIsolated: true, + children: new Set(), + cleanupTasks: new Set(), + server: { clients: [], close: (callback) => callback() }, + }; + const send = vi.fn(); + const session = new CodexSandboxExecSession(execServer, { send, isOpen: () => true }); + const pending = session.handleRequest(request); + try { + await vi.waitFor(() => expect(spawnFault.errors).toHaveLength(1)); + expect(send).not.toHaveBeenCalled(); + expect(finalizeExec).not.toHaveBeenCalled(); + expect(execServer.children.size).toBe(1); + + spawnFault.close?.(); + await pending; + expect(send).toHaveBeenCalledExactlyOnceWith({ + jsonrpc: "2.0", + id: 1, + error: { code: -32603, message: `spawn codex-spawn-resource-fault ${code}` }, + }); + expect(finalizeExec).toHaveBeenCalledExactlyOnceWith({ + status: "failed", + exitCode: null, + timedOut: false, + token: "spawn-failure-token", + }); + expect(execServer.children.size).toBe(0); + await session.close(); + expect(runShellCommand).not.toHaveBeenCalled(); + } finally { + spawnFault.close?.(); + await pending; + await session.close(); + } + }); +}); diff --git a/extensions/codex/src/app-server/sandbox-exec-server.termination.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.termination.test.ts new file mode 100644 index 000000000000..fff9e8fd4c33 --- /dev/null +++ b/extensions/codex/src/app-server/sandbox-exec-server.termination.test.ts @@ -0,0 +1,243 @@ +import { ChildProcess } from "node:child_process"; +import { PassThrough } from "node:stream"; +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; +import { useIsolatedStateGuard } from "openclaw/plugin-sdk/test-env"; +import { withMockedWindowsPlatform } from "openclaw/plugin-sdk/test-node-mocks"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createSandboxContext } from "./sandbox-exec-server.test-helpers.js"; +import { CodexSandboxExecSession } from "./sandbox-exec-server/session.js"; +import type { OpenClawExecServer } from "./sandbox-exec-server/types.js"; + +const spawnMock = vi.hoisted(() => vi.fn()); +vi.mock("node:child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: (...args: Parameters) => { + const child = spawnMock(...args); + void Promise.resolve().then(() => child.emit("spawn")); + return child; + }, + }; +}); + +useIsolatedStateGuard(); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + spawnMock.mockReset(); +}); + +function createFixture() { + const child = Object.assign(new ChildProcess(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + pid: 42_424, + }); + spawnMock.mockReturnValue(child); + const cleanupEntered = createDeferred(); + const releaseCleanup = createDeferred(); + const releaseFinalize = createDeferred(); + const runShellCommand = vi.fn(async () => { + cleanupEntered.resolve(); + await releaseCleanup.promise; + return { code: 0, stdout: Buffer.alloc(0), stderr: Buffer.alloc(0) }; + }); + const finalizeExec = vi.fn(async () => await releaseFinalize.promise); + const sandbox = createSandboxContext({ runShellCommand, finalizeExec }); + if (!sandbox.backend || !sandbox.fsBridge) { + throw new Error("The sandbox fixture requires its backend and filesystem bridge"); + } + const server: OpenClawExecServer = { + environmentId: "termination-test", + authPath: "/termination-test", + refCount: 1, + closed: false, + url: "ws://localhost/termination-test", + sandbox, + backend: sandbox.backend, + fsBridge: sandbox.fsBridge, + networkIsolated: true, + children: new Set(), + cleanupTasks: new Set(), + server: { clients: [], close: (callback) => callback() }, + }; + const send = vi.fn(); + const session = new CodexSandboxExecSession(server, { send, isOpen: () => true }); + return { + child, + cleanupEntered, + releaseCleanup, + releaseFinalize, + runShellCommand, + finalizeExec, + send, + session, + start: () => + session.handleRequest({ + id: 1, + method: "process/start", + params: { processId: "termination", argv: ["ignored"], cwd: "file:///workspace" }, + }), + }; +} + +describe("Codex sandbox local termination authority", () => { + it.skipIf(process.platform === "win32").each( + (["request", "session-close"] as const).flatMap((via) => + (["before-request", "remote-cleanup", "term-grace"] as const).map((exitAt) => ({ + via, + exitAt, + })), + ), + )("stops local signals after exit during $exitAt via $via", async ({ via, exitAt }) => { + vi.useFakeTimers(); + const fixture = createFixture(); + // Keep the real process-tree helper: its detached escalation used to outlive the owner. + const kill = vi.spyOn(process, "kill").mockReturnValue(true); + let cleanup: Promise | undefined; + let completed = false; + const exit = () => { + fixture.child.emit("exit", 7, null); + }; + try { + await fixture.start(); + if (exitAt === "before-request") { + exit(); + await Promise.resolve(); + } + cleanup = ( + via === "request" + ? fixture.session.handleRequest({ + id: 2, + method: "process/terminate", + params: { processId: "termination" }, + }) + : fixture.session.close() + ).then(() => { + completed = true; + }); + await fixture.cleanupEntered.promise; + if (exitAt === "remote-cleanup") { + exit(); + } + fixture.releaseCleanup.resolve(); + await vi.advanceTimersByTimeAsync(0); + if (exitAt === "term-grace") { + expect(kill).toHaveBeenCalledExactlyOnceWith(-fixture.child.pid, "SIGTERM"); + exit(); + } + await vi.advanceTimersByTimeAsync(1_001); + expect(kill.mock.calls).toEqual( + exitAt === "term-grace" ? [[-fixture.child.pid, "SIGTERM"]] : [], + ); + expect(completed).toBe(false); + expect(fixture.finalizeExec).not.toHaveBeenCalled(); + fixture.child.stdout.write("LATE_OUTPUT"); + fixture.child.emit("close", 7, null); + await vi.advanceTimersByTimeAsync(0); + expect(fixture.finalizeExec).toHaveBeenCalledOnce(); + expect(completed).toBe(false); + fixture.releaseFinalize.resolve(); + await cleanup; + expect(fixture.runShellCommand).toHaveBeenCalledOnce(); + expect(fixture.send).toHaveBeenCalledWith({ + jsonrpc: "2.0", + method: "process/output", + params: expect.objectContaining({ chunk: Buffer.from("LATE_OUTPUT").toString("base64") }), + }); + if (via === "request") { + expect(fixture.send).toHaveBeenCalledWith({ + jsonrpc: "2.0", + id: 2, + result: { running: exitAt !== "before-request" }, + }); + } + await vi.advanceTimersByTimeAsync(1_001); + expect(kill.mock.calls).toEqual( + exitAt === "term-grace" ? [[-fixture.child.pid, "SIGTERM"]] : [], + ); + } finally { + fixture.releaseCleanup.resolve(); + fixture.releaseFinalize.resolve(); + fixture.child.emit("close", 7, null); + await Promise.allSettled([cleanup, fixture.session.close()]); + } + }); + + it.skipIf(process.platform === "win32")( + "force kills a still-running child and joins output and backend finalization", + async () => { + vi.useFakeTimers(); + const fixture = createFixture(); + const kill = vi.spyOn(process, "kill").mockReturnValue(true); + let cleanup: Promise | undefined; + try { + await fixture.start(); + cleanup = fixture.session.close(); + await fixture.cleanupEntered.promise; + fixture.releaseCleanup.resolve(); + await vi.advanceTimersByTimeAsync(999); + expect(kill).toHaveBeenCalledExactlyOnceWith(-fixture.child.pid, "SIGTERM"); + await vi.advanceTimersByTimeAsync(1); + expect(kill).toHaveBeenCalledWith(-fixture.child.pid, "SIGKILL"); + expect(fixture.finalizeExec).not.toHaveBeenCalled(); + fixture.child.emit("close", 1, "SIGKILL"); + fixture.releaseFinalize.resolve(); + await cleanup; + expect(fixture.finalizeExec).toHaveBeenCalledOnce(); + } finally { + fixture.releaseCleanup.resolve(); + fixture.releaseFinalize.resolve(); + fixture.child.emit("close", 1, "SIGKILL"); + await Promise.allSettled([cleanup, fixture.session.close()]); + } + }, + ); + + it("joins an admitted Windows taskkill after child close before releasing the backend", async () => { + vi.useFakeTimers(); + const fixture = createFixture(); + const taskkill = new ChildProcess(); + spawnMock.mockImplementation((command) => (command === "taskkill" ? taskkill : fixture.child)); + vi.spyOn(process, "kill").mockReturnValue(true); + let cleanup: Promise | undefined; + let completed = false; + await withMockedWindowsPlatform(async () => { + try { + await fixture.start(); + cleanup = fixture.session.close().then(() => { + completed = true; + }); + await fixture.cleanupEntered.promise; + fixture.releaseCleanup.resolve(); + await vi.advanceTimersByTimeAsync(0); + expect(spawnMock).toHaveBeenCalledWith( + "taskkill", + ["/T", "/PID", String(fixture.child.pid)], + expect.any(Object), + ); + fixture.child.emit("close", 143, "SIGTERM"); + await vi.advanceTimersByTimeAsync(0); + expect(fixture.finalizeExec).not.toHaveBeenCalled(); + expect(completed).toBe(false); + await vi.advanceTimersByTimeAsync(1_001); + expect(spawnMock.mock.calls.filter(([command]) => command === "taskkill")).toHaveLength(1); + taskkill.emit("close", 0); + await vi.advanceTimersByTimeAsync(0); + expect(fixture.finalizeExec).toHaveBeenCalledOnce(); + expect(completed).toBe(false); + fixture.releaseFinalize.resolve(); + await cleanup; + } finally { + fixture.releaseCleanup.resolve(); + fixture.releaseFinalize.resolve(); + fixture.child.emit("close", 143, "SIGTERM"); + taskkill.emit("close", 0); + await Promise.allSettled([cleanup, fixture.session.close()]); + } + }); + }); +}); diff --git a/extensions/codex/src/app-server/sandbox-exec-server.test.ts b/extensions/codex/src/app-server/sandbox-exec-server.test.ts index 41a1a1ccd348..7f4b7bf3d18e 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server.test.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server.test.ts @@ -231,7 +231,6 @@ describe("OpenClaw Codex sandbox exec-server", () => { ); expect(buildExecSpec).toHaveBeenCalledWith( expect.objectContaining({ - command: "'/bin/sh' '-lc' 'printf ok'", env: expect.objectContaining({ CODEX_SANDBOX_EXEC_ID: expect.any(String), POLICY_ONLY: "1", @@ -599,46 +598,164 @@ describe("OpenClaw Codex sandbox exec-server", () => { socket.close(); }); - it("keeps tty process starts pipe-backed for sandbox backends", async () => { - const buildExecSpec = vi.fn(async () => ({ - argv: [process.execPath, "-e", echoFirstInputLineScript("tty:")], - env: testExecEnv(), - stdinMode: "pipe-open" as const, - })); - const sandbox = createSandboxContext({ buildExecSpec }); - const client = createClient(); - await ensureCodexSandboxExecServerEnvironment({ - client: client as never, - sandbox, - }); - const socket = await openSocket(execServerUrlFromClient(client)); - await rpc(socket, "initialize", { clientName: "test" }); - socket.send(JSON.stringify({ method: "initialized" })); + it.runIf(process.platform !== "win32")( + "provides a real terminal and Ctrl-C for tty processes", + async () => { + const buildExecSpec = vi.fn(async () => ({ + argv: [ + process.execPath, + "-e", + [ + "process.on('SIGINT', () => { console.log('INTERRUPTED'); process.exit(42); });", + "console.log('PID=' + process.pid + ' TTY=' + Boolean(process.stdin.isTTY && process.stdout.isTTY));", + "setInterval(() => {}, 1000);", + ].join(" "), + ], + env: testExecEnv(), + stdinMode: "pipe-open" as const, + })); + const sandbox = createSandboxContext({ buildExecSpec }); + const client = createClient(); + await ensureCodexSandboxExecServerEnvironment({ + client: client as never, + sandbox, + }); + const socket = await openSocket(execServerUrlFromClient(client)); + await rpc(socket, "initialize", { clientName: "test" }); + socket.send(JSON.stringify({ method: "initialized" })); - await rpc(socket, "process/start", { - processId: "proc-tty", - argv: ["/bin/sh", "-lc", "cat"], - cwd: "file:///workspace", - env: {}, - tty: true, - pipeStdin: false, - arg0: null, - }); - await expect( - rpc(socket, "process/write", { + await rpc(socket, "process/start", { processId: "proc-tty", - chunk: Buffer.from("hello\n").toString("base64"), - }), - ).resolves.toEqual({ status: "accepted" }); - const read = await readUntilClosed(socket, "proc-tty"); + argv: ["/bin/sh", "-lc", "cat"], + cwd: "file:///workspace", + env: {}, + tty: true, + pipeStdin: false, + arg0: null, + }); + await readStartedPid(socket, "proc-tty"); + const initial = (await rpc(socket, "process/read", { + processId: "proc-tty", + afterSeq: 0, + })) as { + chunks: Array<{ chunk: string }>; + }; + expect( + initial.chunks.map(({ chunk }) => Buffer.from(chunk, "base64").toString()).join(""), + ).toContain("TTY=true"); + await expect( + rpc(socket, "process/write", { + processId: "proc-tty", + chunk: Buffer.from("\u0003").toString("base64"), + }), + ).resolves.toEqual({ status: "accepted" }); + const read = await readUntilClosed(socket, "proc-tty"); - expect(buildExecSpec).toHaveBeenCalledWith(expect.objectContaining({ usePty: false })); - expect(read.chunks?.[0]?.stream).toBe("pty"); - expect(Buffer.from(read.chunks?.[0]?.chunk ?? "", "base64").toString("utf8")).toBe( - "tty:hello\n", - ); - socket.close(); - }); + expect(buildExecSpec).toHaveBeenCalledWith(expect.objectContaining({ usePty: true })); + expect(read.chunks?.[0]?.stream).toBe("pty"); + expect( + read.chunks?.map(({ chunk }) => Buffer.from(chunk, "base64").toString()).join(""), + ).toContain("INTERRUPTED"); + expect(read.exitCode).toBe(42); + socket.close(); + }, + ); + + it.runIf(process.platform !== "win32")( + "interrupts a non-tty process through the sandbox backend", + async () => { + let pid = 0; + let marker = ""; + const sandbox = createSandboxContext({ + buildExecSpec: async ({ env }) => { + marker = `CODEX_SANDBOX_EXEC_ID=${env.CODEX_SANDBOX_EXEC_ID}`; + return { + argv: [ + process.execPath, + "-e", + [ + "process.on('SIGINT', () => { console.log('INTERRUPTED'); process.exit(42); });", + "console.log('PID=' + process.pid); setInterval(() => {}, 1000);", + ].join(" "), + ], + env: testExecEnv(), + stdinMode: "pipe-closed", + }; + }, + runShellCommand: async ({ args, script }) => { + if (script.includes("kill -INT")) { + expect(args).toEqual([marker]); + process.kill(pid, "SIGINT"); + } + return { stdout: Buffer.alloc(0), stderr: Buffer.alloc(0), code: 0 }; + }, + }); + const client = createClient(); + await ensureCodexSandboxExecServerEnvironment({ client: client as never, sandbox }); + const socket = await openSocket(execServerUrlFromClient(client)); + const notifications = collectNotifications(socket); + await rpc(socket, "initialize", {}); + await rpc(socket, "process/start", { + processId: "proc-interrupt", + argv: ["ignored"], + cwd: "file:///workspace", + tty: false, + }); + pid = await readStartedPid(socket, "proc-interrupt"); + await expect( + rpc(socket, "process/signal", { processId: "proc-interrupt", signal: "interrupt" }), + ).resolves.toEqual({}); + const read = await readUntilClosed(socket, "proc-interrupt"); + expect(read.exitCode).toBe(42); + expect( + read.chunks?.map(({ chunk }) => Buffer.from(chunk, "base64").toString()).join(""), + ).toContain("INTERRUPTED"); + expect(notifications).toContainEqual({ + method: "process/exited", + params: expect.objectContaining({ exitCode: 42, sandboxDenied: false }), + }); + await expect( + rpc(socket, "process/signal", { processId: "missing", signal: "interrupt" }), + ).resolves.toEqual({}); + socket.close(); + }, + ); + + it.runIf(process.platform !== "win32")( + "reports a signal-killed PTY as a failed process", + async () => { + const finalizeExec = vi.fn(async () => undefined); + const sandbox = createSandboxContext({ + buildExecSpec: async () => ({ + argv: [process.execPath, "-e", "process.kill(process.pid, 'SIGKILL')"], + env: testExecEnv(), + stdinMode: "pipe-open", + }), + finalizeExec, + }); + const client = createClient(); + await ensureCodexSandboxExecServerEnvironment({ client: client as never, sandbox }); + const socket = await openSocket(execServerUrlFromClient(client)); + const notifications = collectNotifications(socket); + await rpc(socket, "initialize", {}); + await rpc(socket, "process/start", { + processId: "killed-pty", + argv: ["ignored"], + cwd: "file:///workspace", + tty: true, + }); + const read = await readUntilClosed(socket, "killed-pty"); + expect(read.exitCode).toBe(1); + expect(notifications).toContainEqual({ + method: "process/exited", + params: expect.objectContaining({ processId: "killed-pty", exitCode: 1 }), + }); + await vi.waitFor(() => + expect(finalizeExec).toHaveBeenCalledWith(expect.objectContaining({ exitCode: 1 })), + ); + socket.close(); + }, + ); it("does not let Codex env policy inherit host secret variables", async () => { await withEnvAsync( @@ -776,7 +893,7 @@ describe("OpenClaw Codex sandbox exec-server", () => { await rpc(socket, "initialize", { clientName: "test" }); socket.send(JSON.stringify({ method: "initialized" })); - for (const method of ["fs/walk", "process/signal", "unsupported/method"]) { + for (const method of ["fs/walk", "unsupported/method"]) { await expect(rpc(socket, method, {})).rejects.toMatchObject({ code: -32601, message: `Unsupported OpenClaw sandbox exec-server method: ${method}`, diff --git a/extensions/codex/src/app-server/sandbox-exec-server/http.ts b/extensions/codex/src/app-server/sandbox-exec-server/http.ts index d280bdb7b7e2..035fb047b97e 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/http.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/http.ts @@ -3,6 +3,8 @@ * access through the active OpenClaw sandbox backend. */ import { embeddedAgentLog } from "openclaw/plugin-sdk/agent-harness-runtime"; +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; +import { SANDBOX_COMMAND_MAX_BUFFER_BYTES } from "openclaw/plugin-sdk/sandbox"; import { SsrFBlockedError, isBlockedHostnameOrIp } from "openclaw/plugin-sdk/ssrf-runtime"; import { sliceUtf16Safe } from "openclaw/plugin-sdk/text-utility-runtime"; import type { JsonObject, JsonValue } from "../protocol.js"; @@ -10,7 +12,7 @@ import { readHttpHeaders, requireNumber, requireObject, requireString } from "./ import { prepareSandboxChildExec, spawnSandboxChild, - type SandboxChildOwner, + type SandboxPipeChildOwner, } from "./sandbox-child.js"; import type { CodexSandboxExecSessionNotifications, @@ -26,6 +28,7 @@ export async function httpRequest( execServer: OpenClawExecServer, notifications: CodexSandboxExecSessionNotifications, params: JsonValue | undefined, + operations: Set>, ): Promise { const record = requireObject(params, "http/request params"); const requestId = requireString(record.requestId, "requestId"); @@ -47,14 +50,17 @@ export async function httpRequest( redirectPolicy, streamResponse: record.streamResponse === true, }; - if (request.streamResponse) { - return await runStreamingSandboxHttpRequest(execServer, notifications, requestId, request); - } - const result = await runSandboxHttpRequest(execServer, { - ...request, - streamResponse: false, - }); - return result; + const response = createDeferred(); + const operation = runSandboxHttpRequest(execServer, notifications, requestId, request, response); + operations.add(operation); + void operation.then( + () => operations.delete(operation), + (error: unknown) => { + operations.delete(operation); + response.reject(error); + }, + ); + return await response.promise; } type SandboxHttpRequest = { @@ -87,65 +93,60 @@ function assertSandboxHttpRequestTargetAllowed(url: string): void { } async function runSandboxHttpRequest( - execServer: OpenClawExecServer, - params: SandboxHttpRequest, -): Promise { - const result = await execServer.backend.runShellCommand({ - script: SANDBOX_HTTP_REQUEST_SCRIPT, - stdin: JSON.stringify(params), - allowFailure: true, - }); - if (result.code !== 0) { - const stderr = result.stderr.toString("utf8").trim(); - throw new Error(stderr || `sandbox http/request failed with code ${result.code}`); - } - const parsed = JSON.parse(result.stdout.toString("utf8")) as { - status?: unknown; - headers?: unknown; - bodyBase64?: unknown; - }; - if (typeof parsed.status !== "number" || !Array.isArray(parsed.headers)) { - throw new Error("sandbox http/request returned an invalid response envelope"); - } - return { - status: parsed.status, - headers: readHttpHeaders(parsed.headers), - bodyBase64: typeof parsed.bodyBase64 === "string" ? parsed.bodyBase64 : "", - }; -} - -async function runStreamingSandboxHttpRequest( execServer: OpenClawExecServer, notifications: CodexSandboxExecSessionNotifications, requestId: string, params: SandboxHttpRequest, -): Promise { - const backend = execServer.backend; - const remoteExec = prepareSandboxChildExec(backend, {}); - const execSpec = await backend.buildExecSpec({ - command: SANDBOX_HTTP_REQUEST_SCRIPT, - workdir: execServer.sandbox.containerWorkdir, - env: remoteExec.env, - usePty: false, - }); + response: Pick>, "resolve" | "reject">, +): Promise { const lifecycle = { failed: false }; - const owner = await spawnSandboxChild({ - argv: execSpec.argv, - env: execSpec.env, - finalizeExec: backend.finalizeExec, - finalizeToken: execSpec.finalizeToken, - finalizeStatus: (outcome) => - lifecycle.failed || outcome.exitCode !== 0 ? "failed" : "completed", - onFinalizeError: (error) => { - embeddedAgentLog.warn("codex sandbox http/request finalize failed", { error }); - }, - owners: execServer.children, - terminateRemote: remoteExec.terminate, - }); + let owner: SandboxPipeChildOwner; + try { + notifications.signal.throwIfAborted(); + const backend = execServer.backend; + const remoteExec = prepareSandboxChildExec(backend, {}); + const execSpec = await backend.buildExecSpec({ + command: SANDBOX_HTTP_REQUEST_SCRIPT, + workdir: execServer.sandbox.containerWorkdir, + env: remoteExec.env, + usePty: false, + }); + owner = await spawnSandboxChild({ + argv: execSpec.argv, + env: execSpec.env, + cwd: execSpec.cwd, + assertCurrent: () => { + notifications.signal.throwIfAborted(); + execSpec.assertCurrent?.(); + }, + finalizeExec: backend.finalizeExec, + finalizeToken: execSpec.finalizeToken, + finalizeStatus: (outcome) => + lifecycle.failed || outcome.exitCode !== 0 ? "failed" : "completed", + onFinalizeError: (error) => { + embeddedAgentLog.warn("codex sandbox http/request finalize failed", { error }); + }, + owners: execServer.children, + terminateRemote: remoteExec.terminate, + }); + } catch (error) { + response.reject(error); + return; + } const child = owner.process; + const completion = createDeferred(); + void owner.settled.then(() => completion.resolve(), completion.reject); + let termination: Promise | undefined; + const terminate = () => { + if (!termination) { + termination = owner.terminate().then(() => undefined); + void termination.then(completion.resolve, completion.reject); + } + return termination; + }; const abortOnSessionClose = () => { lifecycle.failed = true; - void owner.terminate().catch((error: unknown) => { + void terminate().catch((error: unknown) => { embeddedAgentLog.warn("codex sandbox http/request cleanup failed", { error }); }); }; @@ -153,31 +154,41 @@ async function runStreamingSandboxHttpRequest( child.once("close", () => { notifications.signal.removeEventListener("abort", abortOnSessionClose); }); - if (notifications.signal.aborted) { - abortOnSessionClose(); - } child.stdin.on("error", (error: NodeJS.ErrnoException) => { if (error.code === "EPIPE" || error.code === "ERR_STREAM_DESTROYED") { return; } embeddedAgentLog.warn("codex sandbox http/request stdin write failed", { error }); }); - child.stdin.end(JSON.stringify(params)); - return await readStreamingSandboxHttpResponse({ + void readSandboxHttpResponse({ child, lifecycle, - owner, + terminate, requestId, notifications, - }); + streamResponse: params.streamResponse, + }).then(response.resolve, response.reject); + try { + if (notifications.signal.aborted) { + abortOnSessionClose(); + } else { + child.stdin.end(JSON.stringify(params)); + } + // Headers can finish the RPC while its body or backend finalization is still running. + await completion.promise; + await termination; + } finally { + notifications.signal.removeEventListener("abort", abortOnSessionClose); + } } -function readStreamingSandboxHttpResponse(params: { - child: SandboxChildOwner["process"]; +function readSandboxHttpResponse(params: { + child: SandboxPipeChildOwner["process"]; lifecycle: { failed: boolean }; - owner: SandboxChildOwner; + terminate: () => Promise; requestId: string; notifications: CodexSandboxExecSessionNotifications; + streamResponse: boolean; }): Promise { return new Promise((resolve, reject) => { let headerResolved = false; @@ -186,13 +197,17 @@ function readStreamingSandboxHttpResponse(params: { let lastBodySeq = 0; let stdoutBuffer = ""; let stderr = ""; - const fail = (message: string, _exitCode: number | null) => { + const buffered: Record<"stdout" | "stderr", { chunks: Buffer[]; bytes: number }> = { + stdout: { chunks: [], bytes: 0 }, + stderr: { chunks: [], bytes: 0 }, + }; + const fail = (message: string) => { if (failed) { return; } failed = true; params.lifecycle.failed = true; - void params.owner.terminate().catch((error: unknown) => { + void params.terminate().catch((error: unknown) => { embeddedAgentLog.warn("codex sandbox http/request cleanup failed", { error }); }); if (headerResolved) { @@ -209,9 +224,30 @@ function readStreamingSandboxHttpResponse(params: { } reject(new Error(message)); }; - params.child.stdout.setEncoding("utf8"); - params.child.stdout.on("data", (chunk: string) => { - stdoutBuffer += chunk; + const bufferOutput = (stream: "stdout" | "stderr", chunk: Buffer | string) => { + const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); + const output = buffered[stream]; + output.bytes += buffer.byteLength; + if (output.bytes > SANDBOX_COMMAND_MAX_BUFFER_BYTES) { + fail(`sandbox http/request ${stream} exceeded ${SANDBOX_COMMAND_MAX_BUFFER_BYTES} bytes`); + return; + } + output.chunks.push(buffer); + }; + if (params.streamResponse) { + params.child.stdout.setEncoding("utf8"); + params.child.stderr.setEncoding("utf8"); + } + params.child.stdout.on("data", (chunk: Buffer | string) => { + if (failed) { + return; + } + if (!params.streamResponse) { + bufferOutput("stdout", chunk); + return; + } + const text = typeof chunk === "string" ? chunk : chunk.toString("utf8"); + stdoutBuffer += text; let newline = stdoutBuffer.indexOf("\n"); while (newline >= 0) { const line = stdoutBuffer.slice(0, newline).trim(); @@ -241,7 +277,7 @@ function readStreamingSandboxHttpResponse(params: { } } } catch (error) { - fail(error instanceof Error ? error.message : String(error), null); + fail(error instanceof Error ? error.message : String(error)); } } newline = stdoutBuffer.indexOf("\n"); @@ -249,13 +285,19 @@ function readStreamingSandboxHttpResponse(params: { if (stdoutBuffer.length > SANDBOX_HTTP_STREAM_LINE_MAX_CHARS) { fail( `sandbox http/request produced an unterminated stdout line longer than ${SANDBOX_HTTP_STREAM_LINE_MAX_CHARS} characters`, - null, ); } }); - params.child.stderr.setEncoding("utf8"); - params.child.stderr.on("data", (chunk: string) => { - stderr = sliceUtf16Safe(`${stderr}${chunk}`, -4096); + params.child.stderr.on("data", (chunk: Buffer | string) => { + if (failed) { + return; + } + if (!params.streamResponse) { + bufferOutput("stderr", chunk); + return; + } + const text = typeof chunk === "string" ? chunk : chunk.toString("utf8"); + stderr = sliceUtf16Safe(`${stderr}${text}`, -4096); }); params.child.once("error", (error) => { // ChildProcess error can precede close while the helper is still alive. @@ -269,17 +311,40 @@ function readStreamingSandboxHttpResponse(params: { return; } if (childFailure) { - fail(childFailure, exitCode); + fail(childFailure); return; } if (exitCode === 0) { + if (!params.streamResponse) { + try { + const parsed = JSON.parse(Buffer.concat(buffered.stdout.chunks).toString("utf8")) as { + status?: unknown; + headers?: unknown; + bodyBase64?: unknown; + }; + if (typeof parsed.status !== "number" || !Array.isArray(parsed.headers)) { + throw new Error("sandbox http/request returned an invalid response envelope"); + } + resolve({ + status: parsed.status, + headers: readHttpHeaders(parsed.headers), + bodyBase64: typeof parsed.bodyBase64 === "string" ? parsed.bodyBase64 : "", + }); + } catch (error) { + fail(error instanceof Error ? error.message : String(error)); + } + return; + } if (!headerResolved) { params.lifecycle.failed = true; reject(new Error("sandbox http/request exited before returning headers")); } return; } - fail(stderr.trim() || `sandbox http/request failed with code ${exitCode}`, exitCode); + if (!params.streamResponse) { + stderr = Buffer.concat(buffered.stderr.chunks).toString("utf8"); + } + fail(stderr.trim() || `sandbox http/request failed with code ${exitCode}`); }); }); } diff --git a/extensions/codex/src/app-server/sandbox-exec-server/processes.ts b/extensions/codex/src/app-server/sandbox-exec-server/processes.ts index 6c1eca81c4a6..ba4f0ed3a82e 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/processes.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/processes.ts @@ -123,13 +123,11 @@ async function runProcess( throwIfProcessStartCancelled(managed); const remoteExec = prepareSandboxChildExec(backend, params.env); const execSpec = await backend.buildExecSpec({ - command: buildRemoteCommand(params.argv), + // Preserve the process identity and signal status of the requested argv. + command: `exec ${buildRemoteCommand(params.argv)}`, workdir: params.cwd, env: remoteExec.env, - // This bridge currently owns only pipe-backed child processes. Asking the - // backend for a PTY can produce commands such as `docker exec -t`, which - // require this process itself to own a real TTY. - usePty: false, + usePty: managed.tty, }); if (managed.terminationRequested) { await backend.finalizeExec?.({ @@ -143,6 +141,12 @@ async function runProcess( const owner = await spawnSandboxChild({ argv: execSpec.argv, env: execSpec.env, + cwd: execSpec.cwd, + usePty: managed.tty, + assertCurrent: () => { + execSpec.assertCurrent?.(); + throwIfProcessStartCancelled(managed); + }, finalizeExec: backend.finalizeExec, finalizeToken: execSpec.finalizeToken, finalizeStatus: () => (managed.failure ? "failed" : "completed"), @@ -156,12 +160,17 @@ async function runProcess( }, owners: execServer.children, terminateRemote: remoteExec.terminate, + interruptRemote: remoteExec.interrupt, }); managed.child = owner; + void owner.exited.then(({ exitCode }) => emitProcessExited(managed, exitCode)); + void owner.closed.then(({ exitCode }) => emitProcessClosed(managed, exitCode)); + if ("pty" in owner) { + owner.pty.onData((chunk) => appendProcessChunk(managed, "pty", Buffer.from(chunk))); + return; + } const child = owner.process; - child.stdout.on("data", (chunk: Buffer) => - appendProcessChunk(managed, managed.tty ? "pty" : "stdout", chunk), - ); + child.stdout.on("data", (chunk: Buffer) => appendProcessChunk(managed, "stdout", chunk)); child.stderr.on("data", (chunk: Buffer) => appendProcessChunk(managed, "stderr", chunk)); child.once("error", (error) => { // Node can report an abort or transport error before the child exits. The @@ -169,8 +178,9 @@ async function runProcess( managed.failure ??= error.message; notifyProcessWaiters(managed); }); - child.once("close", (code) => { - emitProcessClosed(managed, code ?? 1); + child.stdin.on("error", (error: Error) => { + managed.failure ??= error.message; + notifyProcessWaiters(managed); }); if (!managed.tty && !managed.pipeStdin) { child.stdin.end(); @@ -217,7 +227,7 @@ function appendProcessChunk( notifyProcessWaiters(managed); } -function emitProcessClosed(managed: ManagedProcess, exitCode: number | null): void { +function emitProcessExited(managed: ManagedProcess, exitCode: number | null): void { if (!managed.exited) { const exitSeq = managed.nextSeq; managed.nextSeq += 1; @@ -228,9 +238,15 @@ function emitProcessClosed(managed: ManagedProcess, exitCode: number | null): vo processId: managed.processId, seq: exitSeq, exitCode, + sandboxDenied: false, }); } } + notifyProcessWaiters(managed); +} + +function emitProcessClosed(managed: ManagedProcess, exitCode: number | null): void { + emitProcessExited(managed, exitCode); if (!managed.closed) { const closeSeq = managed.nextSeq; managed.nextSeq += 1; @@ -276,7 +292,7 @@ export async function readProcess( const managed = requireProcess(processes, processId); const afterSeq = typeof record.afterSeq === "number" ? record.afterSeq : 0; const waitMs = typeof record.waitMs === "number" && record.waitMs > 0 ? record.waitMs : 0; - if (!managed.exited && !hasChunksAtOrAfter(managed, afterSeq) && waitMs > 0) { + if (!managed.closed && managed.nextSeq - 1 <= afterSeq && waitMs > 0) { await waitForProcessUpdate(managed, waitMs); } const chunks = limitProcessChunks( @@ -306,17 +322,37 @@ export function writeProcess( return { status: "unknownProcess" }; } const chunk = Buffer.from(requireString(record.chunk, "chunk"), "base64"); - if ( - (!managed.tty && !managed.pipeStdin) || - managed.closed || - !managed.child?.process.stdin.writable - ) { + if ((!managed.tty && !managed.pipeStdin) || managed.closed || !managed.child) { return { status: "stdinClosed" }; } - managed.child.process.stdin.write(chunk); + if ("pty" in managed.child) { + managed.child.pty.write(chunk); + } else { + if (!managed.child.process.stdin.writable) { + return { status: "stdinClosed" }; + } + managed.child.process.stdin.write(chunk); + } return { status: "accepted" }; } +export async function signalProcess( + processes: Map, + params: JsonValue | undefined, +): Promise { + const record = requireObject(params, "process/signal params"); + const processId = requireString(record.processId, "processId"); + if (record.signal !== "interrupt") { + throw new Error("process/signal only supports interrupt"); + } + const managed = processes.get(processId); + if (managed && !managed.exited) { + await managed.startPromise; + await managed.child?.interrupt(); + } + return {}; +} + /** Requests process termination and reports whether it was running at call time. */ export async function terminateProcess( processes: Map, @@ -359,10 +395,6 @@ function notifyProcessWaiters(managed: ManagedProcess): void { } } -function hasChunksAtOrAfter(managed: ManagedProcess, afterSeq: number): boolean { - return managed.chunks.some((chunk) => chunk.seq > afterSeq); -} - function requireProcess(processes: Map, processId: string): ManagedProcess { const managed = processes.get(processId); if (!managed) { diff --git a/extensions/codex/src/app-server/sandbox-exec-server/sandbox-child.ts b/extensions/codex/src/app-server/sandbox-exec-server/sandbox-child.ts index b5e1b77e899a..267a84b5477b 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/sandbox-child.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/sandbox-child.ts @@ -1,32 +1,63 @@ /** Owns one sandbox subprocess tree through close, reaping, and backend finalization. */ import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { killProcessTree } from "openclaw/plugin-sdk/process-runtime"; +import { once } from "node:events"; +import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; +import { + signalProcessTree, + spawnTerminalPty, + type TerminalPtyHandle, +} from "openclaw/plugin-sdk/process-runtime"; import type { SandboxContext } from "openclaw/plugin-sdk/sandbox"; const SANDBOX_CHILD_TERM_GRACE_MS = 1_000; // Covers the post-TERM tree kill plus Windows taskkill completion before failure is reported. const SANDBOX_CHILD_REAP_TIMEOUT_MS = 4_500; +const SANDBOX_CHILD_INTERRUPT_POLL_MS = 50; +const SANDBOX_REMOTE_PROCESS_PENDING_EXIT_CODE = 75; const SANDBOX_EXEC_MARKER = "CODEX_SANDBOX_EXEC_ID"; -type SandboxChildOutcome = { exitCode: number; signal: NodeJS.Signals | null }; +type SandboxChildOutcome = { exitCode: number; signal: NodeJS.Signals | number | null }; export type SandboxChildOwner = { - process: ChildProcessWithoutNullStreams; + exited: Promise; + closed: Promise; settled: Promise; terminate: () => Promise; }; -export async function spawnSandboxChild(params: { +export type SandboxPipeChildOwner = SandboxChildOwner & { + process: ChildProcessWithoutNullStreams; + interrupt: () => Promise; +}; + +type SandboxPtyChildOwner = SandboxChildOwner & { + pty: TerminalPtyHandle; + interrupt: () => Promise; +}; + +export type SandboxChild = SandboxPipeChildOwner | SandboxPtyChildOwner; + +type SandboxChildStartParams = { argv: string[]; env: NodeJS.ProcessEnv; + cwd?: string; + usePty?: boolean; + assertCurrent?: () => void; finalizeExec?: NonNullable["finalizeExec"]; finalizeToken?: unknown; finalizeStatus: (outcome: SandboxChildOutcome) => "completed" | "failed"; onFinalizeError: (error: unknown) => void; owners: Set; terminateRemote?: () => Promise; -}): Promise { + interruptRemote?: (timeoutMs: number) => Promise; +}; + +export function spawnSandboxChild( + params: SandboxChildStartParams & { usePty?: false }, +): Promise; +export function spawnSandboxChild(params: SandboxChildStartParams): Promise; +export async function spawnSandboxChild(params: SandboxChildStartParams): Promise { const [command, ...args] = params.argv; const finalize = async (status: "completed" | "failed", exitCode: number | null) => await params.finalizeExec?.({ @@ -39,60 +70,101 @@ export async function spawnSandboxChild(params: { await finalize("failed", null).catch(params.onFinalizeError); throw new Error("OpenClaw sandbox exec spec did not provide a command."); } - let child: ChildProcessWithoutNullStreams; - try { - child = spawn(command, args, { - detached: process.platform !== "win32", - env: params.env, - stdio: ["pipe", "pipe", "pipe"], - }); - } catch (error) { - await finalize("failed", null).catch(params.onFinalizeError); - throw error; - } - + let child: ChildProcessWithoutNullStreams | undefined; + let pty: TerminalPtyHandle | undefined; + let exitOutcome: SandboxChildOutcome | undefined; let outcome: SandboxChildOutcome | undefined; - const closed = new Promise((resolve) => { - child.once("close", (code, signal) => resolve((outcome = { exitCode: code ?? 1, signal }))); - }); - let finalizePromise: Promise | undefined; + let escalation: ReturnType | undefined; + const ready = createDeferred(); + const exited = createDeferred(); + const closed = createDeferred(); + const recordExit = (exitCode: number, signal: SandboxChildOutcome["signal"]) => { + if (!exitOutcome) { + clearTimeout(escalation); + exited.resolve((exitOutcome = { exitCode, signal })); + } + }; + const recordClose = (exitCode: number, signal: SandboxChildOutcome["signal"]) => { + recordExit(exitCode, signal); + closed.resolve((outcome = { exitCode, signal })); + }; + let startFailed = false; + let startupPending = true; + let terminationRequested = false; let terminationCleanup: Promise | undefined; let terminationError: Error | undefined; - const settled = closed.then(async (result) => { + let settlementStarted = false; + const interruptions = new Set>(); + const localSignals: Promise[] = []; + const settled = closed.promise.then(async (result) => { + settlementStarted = true; await terminationCleanup; - child.stdin.destroy(); - await (finalizePromise ??= finalize(params.finalizeStatus(result), result.exitCode)); + if (interruptions.size > 0) { + await Promise.allSettled(interruptions); + } + await Promise.all(localSignals); + child?.stdin?.destroy(); + await finalize( + startFailed ? "failed" : params.finalizeStatus(result), + startFailed ? null : result.exitCode, + ); return result; }); void settled.catch(params.onFinalizeError); let terminationPromise: Promise | undefined; const owner: SandboxChildOwner = { - process: child, + exited: exited.promise, + closed: closed.promise, settled, terminate: () => (terminationPromise ??= (async () => { - child.stdin.destroy(); + terminationRequested = true; + if (startupPending) { + await ready.promise; + } + if (startFailed || settlementStarted) { + return await settled; + } + child?.stdin?.destroy(); terminationCleanup = params.terminateRemote?.().catch((error: unknown) => { terminationError = error instanceof Error ? error : new Error(String(error)); }); await terminationCleanup; if (!outcome) { - if (child.pid) { - killProcessTree(child.pid, { - detached: process.platform !== "win32", - graceMs: SANDBOX_CHILD_TERM_GRACE_MS, - }); - } else { - child.kill("SIGTERM"); + const signalLocal = (signal: "SIGTERM" | "SIGKILL") => { + if (exitOutcome) { + return; + } + if (pty) { + pty.kill(signal); + } else if (child?.pid) { + const pid = child.pid; + localSignals.push( + new Promise((resolve) => { + signalProcessTree(pid, signal, { + detached: process.platform !== "win32", + onComplete: resolve, + }); + }), + ); + } else { + child?.kill(signal); + } + }; + signalLocal("SIGTERM"); + if (!exitOutcome) { + escalation = setTimeout(() => signalLocal("SIGKILL"), SANDBOX_CHILD_TERM_GRACE_MS); + escalation.unref?.(); } const reaped = await Promise.race([ - closed.then(() => true), + closed.promise.then(() => true), delay(SANDBOX_CHILD_REAP_TIMEOUT_MS).then(() => false), - ]); + ]).finally(() => clearTimeout(escalation)); + await Promise.all(localSignals); if (!reaped) { throw new Error( - `Sandbox child process tree ${child.pid ?? "unknown"} survived SIGKILL; tear down the sandbox environment and inspect the surviving process tree before retrying.`, + `Sandbox child process tree ${pty?.pid ?? child?.pid ?? "unknown"} survived SIGKILL; tear down the sandbox environment and inspect the surviving process tree before retrying.`, ); } } @@ -108,16 +180,124 @@ export async function spawnSandboxChild(params: { () => params.owners.delete(owner), () => params.owners.delete(owner), ); - return owner; + const assertCurrent = () => { + params.assertCurrent?.(); + if (terminationRequested) { + throw new Error("Sandbox child process start cancelled"); + } + }; + const interrupt = async () => { + await ready.promise; + const interruptRemote = params.interruptRemote; + if (exitOutcome || terminationRequested || !interruptRemote) { + return; + } + const interruption = (async () => { + const deadline = performance.now() + SANDBOX_CHILD_REAP_TIMEOUT_MS; + // The local transport can be ready before the marked process exists on its target. + while (true) { + if (exitOutcome || terminationRequested) { + return; + } + const remainingMs = Math.ceil(deadline - performance.now()); + if (remainingMs <= 0) { + throw new Error( + "Sandbox process interrupt timed out waiting for remote process admission", + ); + } + if (await interruptRemote(remainingMs)) { + return; + } + await Promise.race([ + delay( + Math.min(SANDBOX_CHILD_INTERRUPT_POLL_MS, Math.max(0, deadline - performance.now())), + ), + exited.promise, + ]); + } + })(); + interruptions.add(interruption); + try { + await interruption; + } finally { + interruptions.delete(interruption); + } + }; + try { + if (params.usePty) { + const env = Object.fromEntries( + Object.entries(params.env).filter( + (entry): entry is [string, string] => entry[1] !== undefined, + ), + ); + pty = await spawnTerminalPty( + { file: command, args, cwd: params.cwd, env, cols: 80, rows: 24 }, + { assertCurrent }, + ); + pty.onExit(({ exitCode, signal }) => { + // node-pty leaves exitCode at zero on a signal; native PTY execution reports failure. + recordClose(signal ? 1 : exitCode, signal || null); + }); + return { ...owner, pty, interrupt }; + } + assertCurrent(); + child = spawn(command, args, { + detached: process.platform !== "win32", + env: params.env, + cwd: params.cwd, + stdio: ["pipe", "pipe", "pipe"], + }); + child.once("exit", (code, signal) => recordExit(code ?? 1, signal)); + child.once("close", (code, signal) => recordClose(code ?? 1, signal)); + const markStartFailed = () => { + startFailed = true; + }; + child.once("error", markStartFailed); + try { + await once(child, "spawn"); + } finally { + child.off("error", markStartFailed); + } + return { ...owner, process: child, interrupt }; + } catch (error) { + startFailed = true; + if (!child) { + recordClose(1, null); + } + await settled.catch(() => undefined); + throw error; + } finally { + startupPending = false; + ready.resolve(); + } } export function prepareSandboxChildExec( backend: NonNullable, env: Record, -): { env: Record; terminate: () => Promise } { +): { + env: Record; + terminate: () => Promise; + interrupt: (timeoutMs: number) => Promise; +} { const marker = randomUUID(); return { env: { ...env, [SANDBOX_EXEC_MARKER]: marker }, + interrupt: async (timeoutMs) => { + const result = await backend.runShellCommand({ + script: `${SANDBOX_REMOTE_FIND_OWNED_PIDS}\nowned="$(find_owned_pids "$1")"\n[ -n "$owned" ] || exit ${SANDBOX_REMOTE_PROCESS_PENDING_EXIT_CODE}\nkill -INT $owned 2>/dev/null || true`, + args: [`${SANDBOX_EXEC_MARKER}=${marker}`], + allowFailure: true, + signal: AbortSignal.timeout(timeoutMs), + }); + if (result.code === SANDBOX_REMOTE_PROCESS_PENDING_EXIT_CODE) { + return false; + } + if (result.code !== 0) { + throw new Error(`Sandbox process interrupt failed with code ${result.code}`); + } + return true; + }, terminate: async () => { const result = await backend.runShellCommand({ script: SANDBOX_REMOTE_TERMINATE_SCRIPT, @@ -137,7 +317,7 @@ export function prepareSandboxChildExec( }; } -const SANDBOX_REMOTE_TERMINATE_SCRIPT = String.raw` +const SANDBOX_REMOTE_FIND_OWNED_PIDS = String.raw` find_owned_pids() { for env_file in /proc/[0-9]*/environ; do if [ -r "$env_file" ] && tr '\0' '\n' < "$env_file" 2>/dev/null | grep -Fqx "$1"; then @@ -145,6 +325,10 @@ find_owned_pids() { fi done } +`.trim(); + +const SANDBOX_REMOTE_TERMINATE_SCRIPT = String.raw` +${SANDBOX_REMOTE_FIND_OWNED_PIDS} owned="$(find_owned_pids "$1")" [ -z "$owned" ] || kill -TERM $owned 2>/dev/null || true sleep 1 diff --git a/extensions/codex/src/app-server/sandbox-exec-server/session.ts b/extensions/codex/src/app-server/sandbox-exec-server/session.ts index 05a30f5b664a..5609cae75f7f 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/session.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/session.ts @@ -23,7 +23,13 @@ import { sendError, sendResult, } from "./json-rpc.js"; -import { readProcess, startProcess, terminateProcess, writeProcess } from "./processes.js"; +import { + readProcess, + signalProcess, + startProcess, + terminateProcess, + writeProcess, +} from "./processes.js"; import type { CodexSandboxExecMessageTransport, CodexSandboxExecSessionNotifications, @@ -36,6 +42,7 @@ import type { export class CodexSandboxExecSession { private readonly processes = new Map(); private readonly fileReads: CodexSandboxFileReadHandles = new Map(); + private readonly httpRequests = new Set>(); private readonly closeController = new AbortController(); private readonly notifications: CodexSandboxExecSessionNotifications; private cleanup?: Promise; @@ -85,11 +92,19 @@ export class CodexSandboxExecSession { // Abort streamed HTTP and file reservations before reaping connection-owned processes. this.closeController.abort(); closeAllFileReads(this.fileReads); - this.cleanup = Promise.all( - [...this.processes.keys()].map(async (processId) => + this.cleanup = Promise.allSettled([ + ...this.httpRequests, + ...[...this.processes.keys()].map(async (processId) => terminateProcess(this.processes, { processId }), ), - ).then(() => undefined); + ]).then((results) => { + const failures = results.flatMap((result) => + result.status === "rejected" ? [result.reason] : [], + ); + if (failures.length > 0) { + throw new AggregateError(failures, "Codex sandbox execution cleanup failed"); + } + }); } return this.cleanup; } @@ -114,6 +129,8 @@ export class CodexSandboxExecSession { return await readProcess(this.processes, params); case "process/write": return writeProcess(this.processes, params); + case "process/signal": + return await signalProcess(this.processes, params); case "process/terminate": return await terminateProcess(this.processes, params); case "fs/open": @@ -141,7 +158,7 @@ export class CodexSandboxExecSession { await copyPath(this.execServer, params); return {}; case "http/request": - return await httpRequest(this.execServer, this.notifications, params); + return await httpRequest(this.execServer, this.notifications, params, this.httpRequests); default: throw new JsonRpcProtocolError( JSON_RPC_METHOD_NOT_FOUND, diff --git a/extensions/codex/src/app-server/sandbox-exec-server/types.ts b/extensions/codex/src/app-server/sandbox-exec-server/types.ts index 2ad317cc530e..615a150aff42 100644 --- a/extensions/codex/src/app-server/sandbox-exec-server/types.ts +++ b/extensions/codex/src/app-server/sandbox-exec-server/types.ts @@ -5,7 +5,7 @@ import type { PluginRuntime } from "openclaw/plugin-sdk/plugin-runtime"; import type { SandboxContext } from "openclaw/plugin-sdk/sandbox"; import type { JsonObject, JsonValue } from "../protocol.js"; -import type { SandboxChildOwner } from "./sandbox-child.js"; +import type { SandboxChild, SandboxChildOwner } from "./sandbox-child.js"; /** Minimal JSON-RPC request shape accepted by the sandbox exec-server. */ export type JsonRpcRequest = { @@ -84,7 +84,7 @@ export type ManagedProcess = { tty: boolean; pipeStdin: boolean; terminationRequested: boolean; - child: SandboxChildOwner | null; + child: SandboxChild | null; startPromise?: Promise; evictionTimer?: ReturnType; waiters: Array<() => void>; diff --git a/security/opengrep/precise.yml b/security/opengrep/precise.yml index 2079827c7c7b..7260b42ce5ca 100644 --- a/security/opengrep/precise.yml +++ b/security/opengrep/precise.yml @@ -2450,7 +2450,7 @@ rules: - typescript - javascript severity: WARNING - message: SSH sandbox upload creates a tar stream from a local directory without a preceding symlink boundary validation helper, so tar may follow escaping symlinks before upload. + message: Remote sandbox tar uploads must await assertSafeUploadSymlinks on the uploaded directory before spawning tar, so escaping links are rejected before remote extraction. patterns: - pattern-inside: | async function $FUNC(...){ @@ -2461,13 +2461,13 @@ rules: - pattern: | $SSH = spawn($CMD, $ARGS, ...) - pattern-not-inside: | - async function $FUNC(...){ - ... - await $CHECK($LOCAL); - ... - $TAR = spawn("tar", ["-C", $LOCAL, "-cf", "-", "."], ...) - ... - } + await assertSafeUploadSymlinks($LOCAL); + ... + $TAR = spawn("tar", ["-C", $LOCAL, "-cf", "-", "."], ...) + - pattern-not-inside: | + await assertSafeUploadSymlinks($LOCAL, $SIGNAL); + ... + $TAR = spawn("tar", ["-C", $LOCAL, "-cf", "-", "."], ...) metadata: category: security confidence: medium @@ -2475,7 +2475,9 @@ rules: advisory-url: https://github.com/openclaw/openclaw/security/advisories/GHSA-FV94-QVG8-XQPW detector-bucket: precise source-run: 2026-04-17T07-37-10Z + advisory-id: GHSA-FV94-QVG8-XQPW source-rule-id: ssh-sandbox-upload-missing-symlink-boundary-check + source-file: security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.yml - id: ghsa-g353-mgv3-8pcj.feishu-webhook-mode-missing-encrypt-key message: Feishu/Lark webhook-mode configuration sets verificationToken without also configuring encryptKey. severity: ERROR diff --git a/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.js b/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.js new file mode 100644 index 000000000000..dc02f425cb17 --- /dev/null +++ b/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.js @@ -0,0 +1,101 @@ +// Static OpenGrep fixtures. These functions are never executed. +import { spawn } from "node:child_process"; + +async function validatedWithoutSignal(params) { + await assertSafeUploadSymlinks(params.localDir); + await new Promise((resolve, reject) => { + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validatedWithSignal(params) { + await assertSafeUploadSymlinks(params.localDir, params.signal); + await new Promise((resolve, reject) => { + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], { signal: params.signal }); + const remote = spawn("ssh", ["fixture"], { signal: params.signal }); + }); +} + +async function validatedDirectSpawn(params) { + await assertSafeUploadSymlinks(params.localDir, params.signal); + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); +} + +async function laterValidationDoesNotAuthorizeEarlierUpload(params) { + let tar; + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + await assertSafeUploadSymlinks(params.localDir); + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); +} + +async function missingValidation(params) { + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validationNotAwaited(params) { + assertSafeUploadSymlinks(params.localDir, params.signal); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validatesOtherDirectory(params, otherDir) { + await assertSafeUploadSymlinks(otherDir, params.signal); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validatesAfterUpload(params) { + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); + await assertSafeUploadSymlinks(params.localDir, params.signal); +} + +async function arbitraryChecker(params) { + await inspectDirectory(params.localDir); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function conditionalValidation(params, validate) { + if (validate) { + await assertSafeUploadSymlinks(params.localDir, params.signal); + } + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function swallowedValidationFailure(params) { + await assertSafeUploadSymlinks(params.localDir, params.signal).catch(() => {}); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} diff --git a/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.ts b/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.ts new file mode 100644 index 000000000000..5de0496ce3ed --- /dev/null +++ b/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.ts @@ -0,0 +1,107 @@ +// Static OpenGrep fixtures. These functions are never executed. +import { spawn, type ChildProcess } from "node:child_process"; + +type Upload = { localDir: string; signal?: AbortSignal }; +declare function assertSafeUploadSymlinks(localDir: string, signal?: AbortSignal): Promise; +declare function inspectDirectory(localDir: string): Promise; + +async function validatedWithoutSignal(params: Upload) { + await assertSafeUploadSymlinks(params.localDir); + await new Promise((resolve, reject) => { + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validatedWithSignal(params: Upload) { + await assertSafeUploadSymlinks(params.localDir, params.signal); + await new Promise((resolve, reject) => { + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + const tar: ChildProcess = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], { + signal: params.signal, + }); + const remote: ChildProcess = spawn("ssh", ["fixture"], { signal: params.signal }); + }); +} + +async function validatedDirectSpawn(params: Upload) { + await assertSafeUploadSymlinks(params.localDir, params.signal); + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); +} + +async function laterValidationDoesNotAuthorizeEarlierUpload(params: Upload) { + let tar: ChildProcess; + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + await assertSafeUploadSymlinks(params.localDir); + // ok: ssh-sandbox-upload-missing-symlink-boundary-check + tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); +} + +async function missingValidation(params: Upload) { + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validationNotAwaited(params: Upload) { + assertSafeUploadSymlinks(params.localDir, params.signal); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validatesOtherDirectory(params: Upload, otherDir: string) { + await assertSafeUploadSymlinks(otherDir, params.signal); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function validatesAfterUpload(params: Upload) { + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); + await assertSafeUploadSymlinks(params.localDir, params.signal); +} + +async function arbitraryChecker(params: Upload) { + await inspectDirectory(params.localDir); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function conditionalValidation(params: Upload, validate: boolean) { + if (validate) { + await assertSafeUploadSymlinks(params.localDir, params.signal); + } + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} + +async function swallowedValidationFailure(params: Upload) { + await assertSafeUploadSymlinks(params.localDir, params.signal).catch(() => {}); + await new Promise((resolve, reject) => { + // ruleid: ssh-sandbox-upload-missing-symlink-boundary-check + const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], {}); + const remote = spawn("ssh", ["fixture"], {}); + }); +} diff --git a/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.yml b/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.yml new file mode 100644 index 000000000000..ddd7e41b1603 --- /dev/null +++ b/security/opengrep/rules/ghsa-fv94-qvg8-xqpw/ssh-sandbox-upload.yml @@ -0,0 +1,32 @@ +rules: + - id: ssh-sandbox-upload-missing-symlink-boundary-check + languages: + - typescript + - javascript + severity: WARNING + message: Remote sandbox tar uploads must await assertSafeUploadSymlinks on the uploaded directory before spawning tar, so escaping links are rejected before remote extraction. + patterns: + - pattern-inside: | + async function $FUNC(...){ + ... + } + - pattern: | + $TAR = spawn("tar", ["-C", $LOCAL, "-cf", "-", "."], ...) + - pattern: | + $SSH = spawn($CMD, $ARGS, ...) + # A later safe upload must not exempt an earlier unvalidated spawn. + - pattern-not-inside: | + await assertSafeUploadSymlinks($LOCAL); + ... + $TAR = spawn("tar", ["-C", $LOCAL, "-cf", "-", "."], ...) + - pattern-not-inside: | + await assertSafeUploadSymlinks($LOCAL, $SIGNAL); + ... + $TAR = spawn("tar", ["-C", $LOCAL, "-cf", "-", "."], ...) + metadata: + category: security + confidence: medium + ghsa: GHSA-FV94-QVG8-XQPW + advisory-url: https://github.com/openclaw/openclaw/security/advisories/GHSA-FV94-QVG8-XQPW + detector-bucket: precise + source-run: 2026-04-17T07-37-10Z diff --git a/src/agents/sandbox/remote-shell-backend.test.ts b/src/agents/sandbox/remote-shell-backend.test.ts index 79f6d26c8ca9..0fdaf5dab7e5 100644 --- a/src/agents/sandbox/remote-shell-backend.test.ts +++ b/src/agents/sandbox/remote-shell-backend.test.ts @@ -1,6 +1,6 @@ import fs from "node:fs/promises"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createDeferred } from "../../../test/helpers/promise.js"; import { useAutoCleanupTempDirTracker } from "../../../test/helpers/temp-dir.js"; import { hasErrnoCode } from "../../infra/errno.js"; @@ -15,6 +15,156 @@ type RemoteShellUploadParams = Parameters(async () => ({ + stdout: Buffer.from("1\n"), + stderr: Buffer.alloc(0), + code: 0, + })), + uploadDirectory: vi.fn(async () => {}), + prepareExec: async () => { + throw new Error("unexpected exec preparation"); + }, + dispose: vi.fn(async () => {}), + } satisfies RemoteShellSandboxSession; + const createSession = vi.fn(async () => session); + const backend = await createRemoteShellSandboxBackend( + { + cfg: resolveSandboxConfigForAgent({ + agents: { + defaults: { + sandbox: { + mode: "all", + backend: "ssh", + workspaceAccess: "rw", + ssh: { target: "unused", workspaceRoot: path.join(workspaceDir, "remote") }, + }, + }, + }, + }), + scopeKey: "command-cancellation", + sessionKey: "test", + workspaceDir, + agentWorkspaceDir: workspaceDir, + skillsWorkspaceDir: workspaceDir, + }, + { createSession }, + ); + await backend.runShellCommand({ script: "true" }); + session.runCommand.mockClear(); + session.uploadDirectory.mockClear(); + session.dispose.mockClear(); + createSession.mockClear(); + return { backend, session, createSession }; +} + +describe("remote shell command cancellation", () => { + it.each(["clear", "upload"] as const)( + "cancels the skills %s and joins it and session disposal before rejecting", + async (phase) => { + const { backend, session } = await createCommandCancellationFixture(); + const controller = new AbortController(); + const entered = createDeferred(); + const release = createDeferred(); + const disposing = createDeferred(); + const releaseDisposal = createDeferred(); + let cancellationObserved = false; + let completed = false; + const hold = async (signal?: AbortSignal) => { + signal?.addEventListener( + "abort", + () => { + cancellationObserved = true; + }, + { once: true }, + ); + entered.resolve(); + await release.promise; + signal?.throwIfAborted(); + }; + if (phase === "clear") { + session.runCommand.mockImplementationOnce(async ({ signal }) => { + await hold(signal); + return { stdout: Buffer.alloc(0), stderr: Buffer.alloc(0), code: 0 }; + }); + } else { + session.uploadDirectory.mockImplementationOnce(({ signal }) => hold(signal)); + } + session.dispose.mockImplementationOnce(async () => { + disposing.resolve(); + await releaseDisposal.promise; + }); + const command = backend.runShellCommand({ + script: "touch sentinel", + signal: controller.signal, + }); + const result = command.then( + () => { + completed = true; + return undefined; + }, + (error: unknown) => { + completed = true; + return error; + }, + ); + try { + await entered.promise; + controller.abort(new Error("interrupt deadline")); + expect(cancellationObserved).toBe(true); + expect(session.dispose).not.toHaveBeenCalled(); + expect(completed).toBe(false); + release.resolve(); + await disposing.promise; + expect(completed).toBe(false); + releaseDisposal.resolve(); + expect(await result).toEqual(expect.objectContaining({ message: "interrupt deadline" })); + expect(session.runCommand).toHaveBeenCalledOnce(); + expect(session.uploadDirectory).toHaveBeenCalledTimes(phase === "upload" ? 1 : 0); + expect(session.dispose).toHaveBeenCalledOnce(); + } finally { + release.resolve(); + releaseDisposal.resolve(); + await result; + } + }, + ); + + it("disposes a session acquired after cancellation without starting remote work", async () => { + const { backend, session, createSession } = await createCommandCancellationFixture(); + const entered = createDeferred(); + const release = createDeferred(); + const controller = new AbortController(); + createSession.mockImplementationOnce(async () => { + entered.resolve(); + await release.promise; + return session; + }); + const command = backend.runShellCommand({ + script: "touch sentinel", + signal: controller.signal, + }); + const result = command.then( + () => undefined, + (error: unknown) => error, + ); + try { + await entered.promise; + controller.abort(new Error("interrupt deadline")); + release.resolve(); + expect(await result).toEqual(expect.objectContaining({ message: "interrupt deadline" })); + expect(session.runCommand).not.toHaveBeenCalled(); + expect(session.uploadDirectory).not.toHaveBeenCalled(); + expect(session.dispose).toHaveBeenCalledOnce(); + } finally { + release.resolve(); + await result; + } + }); +}); + async function createFixture() { const root = await fs.realpath(tempDirs.make("remote-shell-bootstrap-")); const remoteRoot = path.join(root, "remote"); diff --git a/src/agents/sandbox/remote-shell-backend.ts b/src/agents/sandbox/remote-shell-backend.ts index e02b2cd9c164..afb8b68680e7 100644 --- a/src/agents/sandbox/remote-shell-backend.ts +++ b/src/agents/sandbox/remote-shell-backend.ts @@ -341,7 +341,10 @@ class RemoteShellSandboxBackendImpl { ); } - private async refreshRemoteSkillsWorkspace(session: RemoteShellSandboxSession): Promise { + private async refreshRemoteSkillsWorkspace( + session: RemoteShellSandboxSession, + signal?: AbortSignal, + ): Promise { if ( this.params.preprovisionedWorkdir || this.params.createParams.cfg.workspaceAccess !== "rw" || @@ -349,8 +352,14 @@ class RemoteShellSandboxBackendImpl { ) { return; } - await this.clearRemoteDirectory(session, this.params.runtimePaths.remoteSkillsWorkspaceDir); - if (!(await isExistingDirectory(this.params.createParams.skillsWorkspaceDir))) { + await this.clearRemoteDirectory( + session, + this.params.runtimePaths.remoteSkillsWorkspaceDir, + signal, + ); + const hasSkills = await isExistingDirectory(this.params.createParams.skillsWorkspaceDir); + signal?.throwIfAborted(); + if (!hasSkills) { return; } this.params.createParams.assertRuntimeCurrent?.(); @@ -358,13 +367,16 @@ class RemoteShellSandboxBackendImpl { localDir: this.params.createParams.skillsWorkspaceDir, remoteDir: this.params.runtimePaths.remoteSkillsWorkspaceDir, remoteRootDir: this.params.runtimePaths.runtimeRootDir, + signal, }); } private async clearRemoteDirectory( session: RemoteShellSandboxSession, remoteDir: string, + signal?: AbortSignal, ): Promise { + signal?.throwIfAborted(); this.params.createParams.assertRuntimeCurrent?.(); await session.runCommand({ remoteCommand: buildRemoteCommand([ @@ -375,16 +387,21 @@ class RemoteShellSandboxBackendImpl { remoteDir, this.params.runtimePaths.runtimeRootDir, ]), + signal, }); } async runRemoteShellScript( params: SandboxBackendCommandParams, ): Promise { + params.signal?.throwIfAborted(); await this.ensureRuntime(); + params.signal?.throwIfAborted(); const session = await this.createSession(); try { - await this.refreshRemoteSkillsWorkspace(session); + params.signal?.throwIfAborted(); + await this.refreshRemoteSkillsWorkspace(session, params.signal); + params.signal?.throwIfAborted(); this.params.createParams.assertRuntimeCurrent?.(); return await session.runCommand({ remoteCommand: buildRemoteCommand([ diff --git a/src/agents/sandbox/remote-shell-transport.ts b/src/agents/sandbox/remote-shell-transport.ts index 8f3b5ccf5755..6ea0596aab76 100644 --- a/src/agents/sandbox/remote-shell-transport.ts +++ b/src/agents/sandbox/remote-shell-transport.ts @@ -1,5 +1,5 @@ /** Remote-shell transport operations shared by SSH and provider-owned execution. */ -import { spawn } from "node:child_process"; +import { spawn, type ChildProcess } from "node:child_process"; import { randomUUID } from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; @@ -71,6 +71,7 @@ export function createRemoteShellSandboxSession( if (checkCurrent) { options.assertCurrent?.(); } + params.signal?.throwIfAborted(); const result = await spawnCommand(command.argv, { baseEnv: command.env, cwd: command.cwd, @@ -185,7 +186,7 @@ async function uploadDirectoryToRemoteCommand( params: RemoteShellUploadParams, options: RemoteShellSessionOptions, ): Promise { - await assertSafeUploadSymlinks(params.localDir); + await assertSafeUploadSymlinks(params.localDir, params.signal); const remoteCommand = buildRemoteCommand([ "/bin/sh", "-c", @@ -202,12 +203,13 @@ async function uploadDirectoryToRemoteCommand( const tarEnv = sanitizeEnvVars(process.env).allowed; await new Promise((resolve, reject) => { options.assertCurrent?.(); - const tar = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], { + params.signal?.throwIfAborted(); + const tar: ChildProcess = spawn("tar", ["-C", params.localDir, "-cf", "-", "."], { stdio: ["ignore", "pipe", "pipe"], env: tarEnv, signal: params.signal, }); - const remote = spawn(executable, args, { + const remote: ChildProcess = spawn(executable, args, { stdio: ["pipe", "pipe", "pipe"], env: command.env, cwd: command.cwd, @@ -222,13 +224,15 @@ async function uploadDirectoryToRemoteCommand( let remoteCode: number | null = 0; let tarSignal: NodeJS.Signals | null = null; let remoteSignal: NodeJS.Signals | null = null; + let failure: Error | undefined; let settled = false; const fail = (error: unknown) => { - if (settled) { + if (settled || failure) { return; } - settled = true; + // Abort and stream errors can precede close; cleanup must still join both children. + failure = toErrorObject(error, "Non-Error rejection"); for (const child of [tar, remote]) { try { child.kill("SIGKILL"); @@ -236,18 +240,9 @@ async function uploadDirectoryToRemoteCommand( // Preserve the pipeline error while still terminating the peer. } } - reject(toErrorObject(error, "Non-Error rejection")); + maybeResolve(); }; - tar.stderr.on("data", (chunk) => tarStderr.push(Buffer.from(chunk))); - tar.stderr.on("error", fail); - tar.stdout.on("error", fail); - remote.stdout.on("data", (chunk) => remoteStdout.push(Buffer.from(chunk))); - remote.stdout.on("error", fail); - remote.stderr.on("data", (chunk) => remoteStderr.push(Buffer.from(chunk))); - remote.stderr.on("error", fail); - remote.stdin?.on("error", fail); - tar.on("error", fail); remote.on("error", fail); @@ -264,11 +259,25 @@ async function uploadDirectoryToRemoteCommand( maybeResolve(); }); + // EMFILE/ENFILE can leave streams absent; native error and close still settle the child. + tar.stderr?.on("data", (chunk) => tarStderr.push(Buffer.from(chunk))); + tar.stderr?.on("error", fail); + tar.stdout?.on("error", fail); + remote.stdout?.on("data", (chunk) => remoteStdout.push(Buffer.from(chunk))); + remote.stdout?.on("error", fail); + remote.stderr?.on("data", (chunk) => remoteStderr.push(Buffer.from(chunk))); + remote.stderr?.on("error", fail); + remote.stdin?.on("error", fail); + function maybeResolve() { if (settled || !tarClosed || !remoteClosed) { return; } settled = true; + if (failure) { + reject(failure); + return; + } // A null code means the process died from a signal (OOM kill, dropped // connection, supervisor teardown) without reporting a status. An // unknown outcome is not evidence of a completed transfer. @@ -302,20 +311,24 @@ async function uploadDirectoryToRemoteCommand( try { // Readable pipe errors do not close the writable peer automatically. - tar.stdout.pipe(remote.stdin); + if (tar.stdout && remote.stdin) { + tar.stdout.pipe(remote.stdin); + } } catch (error) { fail(error); } }); } -async function assertSafeUploadSymlinks(localDir: string): Promise { +async function assertSafeUploadSymlinks(localDir: string, signal?: AbortSignal): Promise { const rootDir = path.resolve(localDir); await walkDirectory(rootDir); async function walkDirectory(currentDir: string): Promise { + signal?.throwIfAborted(); const entries = await fs.readdir(currentDir, { withFileTypes: true }); for (const entry of entries) { + signal?.throwIfAborted(); const entryPath = path.join(currentDir, entry.name); if (entry.isSymbolicLink()) { // The remote tar extract should not recreate links that escape the diff --git a/src/agents/sandbox/ssh.spawn-env.test.ts b/src/agents/sandbox/ssh.spawn-env.test.ts index ee10712b06e5..6b2f60fa1cbf 100644 --- a/src/agents/sandbox/ssh.spawn-env.test.ts +++ b/src/agents/sandbox/ssh.spawn-env.test.ts @@ -297,35 +297,46 @@ describe("ssh subprocess env sanitization", () => { ).rejects.toThrow("ssh stream failed"); }); - it("does not spawn an upload after authority is revoked during local traversal", async () => { - let current = true; - const localDir = ownedDirs.make("openclaw-ssh-upload-admission-"); - await fs.writeFile(path.join(localDir, "payload.txt"), "synthetic payload"); - spawnMock.mockImplementation(() => { - throw new Error("unexpected native spawn"); - }); - try { - const uploading = uploadDirectoryToSshTarget({ - session: { - command: "ssh", - configPath: "/tmp/openclaw-test-ssh-config", - host: "openclaw-sandbox", - assertCurrent: () => { - if (!current) { - throw new Error("runtime removed"); - } - }, - }, - localDir, - remoteDir: "/remote/workspace", + it.each(["authority revocation", "cancellation"] as const)( + "does not spawn an upload after %s during local traversal", + async (reason) => { + let current = true; + const controller = new AbortController(); + const localDir = ownedDirs.make("openclaw-ssh-upload-admission-"); + await fs.writeFile(path.join(localDir, "payload.txt"), "synthetic payload"); + spawnMock.mockImplementation(() => { + throw new Error("unexpected native spawn"); }); - current = false; - await expect(uploading).rejects.toThrow("runtime removed"); - expect(spawnMock).not.toHaveBeenCalled(); - } finally { - spawnMock.mockReset(); - } - }); + try { + const uploading = uploadDirectoryToSshTarget({ + session: { + command: "ssh", + configPath: "/tmp/openclaw-test-ssh-config", + host: "openclaw-sandbox", + assertCurrent: () => { + if (!current) { + throw new Error("runtime removed"); + } + }, + }, + localDir, + remoteDir: "/remote/workspace", + signal: controller.signal, + }); + if (reason === "authority revocation") { + current = false; + } else { + controller.abort(new Error("upload cancelled")); + } + await expect(uploading).rejects.toThrow( + reason === "authority revocation" ? "runtime removed" : "upload cancelled", + ); + expect(spawnMock).not.toHaveBeenCalled(); + } finally { + spawnMock.mockReset(); + } + }, + ); it("filters blocked secrets before spawning ssh uploads", async () => { mockSuccessfulSpawnCalls(2); diff --git a/src/agents/sandbox/ssh.stream-errors.test.ts b/src/agents/sandbox/ssh.stream-errors.test.ts index 24d4d56ce9e4..8973a858a625 100644 --- a/src/agents/sandbox/ssh.stream-errors.test.ts +++ b/src/agents/sandbox/ssh.stream-errors.test.ts @@ -55,9 +55,79 @@ function fakeSession(): import("./ssh.js").SshSandboxSession { } describe("SSH sandbox stream errors", () => { - it.each(["tar.stdout", "tar.stderr", "ssh.stdin", "ssh.stdout", "ssh.stderr"] as const)( - "rejects and terminates both upload children once when %s fails", - async (stream) => { + it.each([ + { process: "tar", code: "EMFILE" }, + { process: "tar", code: "ENFILE" }, + { process: "ssh", code: "EMFILE" }, + { process: "ssh", code: "ENFILE" }, + ] as const)( + "preserves $process $code without streams and waits for both children to close", + async ({ process: childName, code }) => { + const failed = Object.assign(new EventEmitter(), { kill: vi.fn(() => false) }); + const peer = createMockChildProcess(); + const tar = childName === "tar" ? failed : peer; + const ssh = childName === "ssh" ? failed : peer; + const nativeError = Object.assign(new Error(`spawn ${childName} ${code}`), { code }); + const errorEmitted = createDeferred(); + // Keep the intentionally broken baseline from crashing this test worker. + failed.on("error", () => errorEmitted.resolve()); + const returnChild = (child: typeof failed | MockChildProcess) => { + if (child === failed) { + queueMicrotask(() => failed.emit("error", nativeError)); + } + return child; + }; + spawnMock + .mockImplementationOnce(() => returnChild(tar)) + .mockImplementationOnce(() => returnChild(ssh)); + let completed = false; + const result = uploadDirectoryToSshTarget({ + session: fakeSession(), + localDir, + remoteDir: "/remote/workspace", + }).then( + () => { + completed = true; + return undefined; + }, + (error: unknown) => { + completed = true; + return error; + }, + ); + try { + await withTestTimeout(errorEmitted.promise, 10_000, "native spawn error did not arrive"); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(completed).toBe(false); + expect(peer.kill).toHaveBeenCalledExactlyOnceWith("SIGKILL"); + failed.emit("close", code === "EMFILE" ? -24 : -23, null); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(completed).toBe(false); + peer.emit("close", null, "SIGKILL"); + expect(await result).toBe(nativeError); + } finally { + failed.emit("close", code === "EMFILE" ? -24 : -23, null); + peer.emit("close", null, "SIGKILL"); + await result; + } + }, + ); + + it.each([ + { process: "tar", stream: "stdout" }, + { process: "tar", stream: "stderr" }, + { process: "ssh", stream: "stdin" }, + { process: "ssh", stream: "stdout" }, + { process: "ssh", stream: "stderr" }, + { process: "tar", stream: "error" }, + { process: "ssh", stream: "error" }, + ] as const)( + "reaps both upload children before rejecting $process $stream failure", + async ({ process: childName, stream: streamName }) => { const tar = createMockChildProcess(); const ssh = createMockChildProcess(); const childrenSpawned = createDeferred(); @@ -65,7 +135,8 @@ describe("SSH sandbox stream errors", () => { childrenSpawned.resolve(); return ssh as unknown as ChildProcess; }); - const expected = `${stream} failed`; + const expected = `${childName}.${streamName} failed`; + let completed = false; const result = uploadDirectoryToSshTarget({ session: fakeSession(), localDir, @@ -76,6 +147,7 @@ describe("SSH sandbox stream errors", () => { throw new Error(`expected rejection: ${expected}`); }, (error: unknown) => { + completed = true; expect(error).toEqual(expect.objectContaining({ message: expected })); }, ); @@ -85,18 +157,32 @@ describe("SSH sandbox stream errors", () => { "tar/ssh upload children did not spawn", ); expect(spawnMock).toHaveBeenCalledTimes(2); - const [childName, streamName] = stream.split(".") as ["tar" | "ssh", keyof MockChildProcess]; - const failedStream = { tar, ssh }[childName][streamName] as PassThrough; + const failedChild = { tar, ssh }[childName]; + const emitError = (message: string) => { + if (streamName === "error") { + failedChild.emit("error", new Error(message)); + } else { + failedChild[streamName].emit("error", new Error(message)); + } + }; - failedStream.emit("error", new Error(expected)); - - await rejection; + emitError(expected); + emitError("later upload failure"); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(completed).toBe(false); expect(tar.kill).toHaveBeenCalledExactlyOnceWith("SIGKILL"); expect(ssh.kill).toHaveBeenCalledExactlyOnceWith("SIGKILL"); - tar.emit("close", 0); - ssh.emit("close", 0); - failedStream.emit("error", new Error("late stream error")); + tar.emit("close", null, "SIGKILL"); + await new Promise((resolve) => { + setImmediate(resolve); + }); + expect(completed).toBe(false); + ssh.emit("close", null, "SIGKILL"); + await rejection; + emitError("late stream error"); expect(tar.kill).toHaveBeenCalledOnce(); expect(ssh.kill).toHaveBeenCalledOnce(); }, diff --git a/src/gateway/server-methods/chat-send-agent-dispatch.ts b/src/gateway/server-methods/chat-send-agent-dispatch.ts index 4a8333eeaa16..60c2e4723099 100644 --- a/src/gateway/server-methods/chat-send-agent-dispatch.ts +++ b/src/gateway/server-methods/chat-send-agent-dispatch.ts @@ -518,7 +518,6 @@ export function startChatDispatch(params: StartChatDispatchParams): void { ) .then(async (dispatchResult) => { if (acceptedMessageInjection) { - dispatchErrorLifecycle.recordAbortedResult(); return; } emitServerTiming("dispatch-completed", undefined, dispatchStartedAtMs); @@ -666,8 +665,6 @@ export function startChatDispatch(params: StartChatDispatchParams): void { ...(returnedAgentError ? { error: returnedAgentError } : {}), }, }); - } else { - dispatchErrorLifecycle.recordAbortedResult(); } }, { diff --git a/src/gateway/server-methods/chat-send-dispatch-errors.test.ts b/src/gateway/server-methods/chat-send-dispatch-errors.test.ts index 15d1149d95b5..436c67ac4a22 100644 --- a/src/gateway/server-methods/chat-send-dispatch-errors.test.ts +++ b/src/gateway/server-methods/chat-send-dispatch-errors.test.ts @@ -359,115 +359,120 @@ describe("createChatSendDispatchErrorLifecycle", () => { expect(removeChatRun).toHaveBeenCalledWith("run-1", "run-1", "agent:main:main"); }); - it("preserves an explicitly aborted terminal when its dispatch later rejects", async () => { - const runId = "explicit-abort-before-dispatch-rejection"; - const sessionKey = "agent:main:main"; - const chatAbortControllers = new Map(); - const chatRunState = createChatRunState(); - const registration = registerChatAbortController({ - chatAbortControllers, - runId, - sessionId: "sess-main", - sessionKey, - timeoutMs: 60_000, - }); - if (!registration.registered) { - throw new Error("expected the chat abort controller to be registered"); - } - const entry = registration.entry; - const removeChatRun = vi.fn(); - const broadcast = vi.fn(); - const dedupe = new Map(); - const warn = vi.fn(); - const terminalizeRestartSafeAdmission = vi.fn(); - const unsubscribe = onAgentRuntimeEvent((event) => { - if (event.runId !== runId || event.stream !== "lifecycle" || event.data.phase !== "end") { - return; + it.each(["resolves", "rejects"])( + "preserves an explicitly aborted terminal when its dispatch later %s", + async (settlement) => { + const runId = `explicit-abort-before-dispatch-${settlement}`; + const sessionKey = "agent:main:main"; + const chatAbortControllers = new Map(); + const chatRunState = createChatRunState(); + const registration = registerChatAbortController({ + chatAbortControllers, + runId, + sessionId: "sess-main", + sessionKey, + timeoutMs: 60_000, + }); + if (!registration.registered) { + throw new Error("expected the chat abort controller to be registered"); } - const current = chatAbortControllers.get(runId); - if (current) { - current.projectSessionTerminalPending = true; - current.projectSessionTerminalObservedAt = event.ts; - } - }); + const entry = registration.entry; + const removeChatRun = vi.fn(); + const broadcast = vi.fn(); + const dedupe = new Map(); + const warn = vi.fn(); + const terminalizeRestartSafeAdmission = vi.fn(); + const unsubscribe = onAgentRuntimeEvent((event) => { + if (event.runId !== runId || event.stream !== "lifecycle" || event.data.phase !== "end") { + return; + } + const current = chatAbortControllers.get(runId); + if (current) { + current.projectSessionTerminalPending = true; + current.projectSessionTerminalObservedAt = event.ts; + } + }); - try { - expect( - abortChatRunById( - { - chatAbortControllers, - chatRunState, - removeChatRun, + try { + expect( + abortChatRunById( + { + chatAbortControllers, + chatRunState, + removeChatRun, + agentRunSeq: new Map(), + broadcast, + nodeSendToSession: vi.fn(), + }, + { runId, sessionKey }, + ), + ).toEqual({ aborted: true }); + + const lifecycle = createChatSendDispatchErrorLifecycle({ + admission: { + sessionBinding: { + sessionId: "sess-main", + sessionKey: "agent:main:main", + agentId: "main", + lifecycleGeneration: "test-generation", + }, + activeRunAbort: registration, + cleanupAdmittedRun: registration.cleanup, + lifecycleGeneration: "test-generation", + restartSafeAdmission: {} as never, + }, + context: { agentRunSeq: new Map(), broadcast, + chatRunState, + dedupe, + getRuntimeConfig: () => ({}), + logGateway: { warn }, nodeSendToSession: vi.fn(), - }, - { runId, sessionKey }, - ), - ).toEqual({ aborted: true }); - - const lifecycle = createChatSendDispatchErrorLifecycle({ - admission: { - sessionBinding: { - sessionId: "sess-main", - sessionKey: "agent:main:main", + removeChatRun, + } as never, + isQueuedFollowupEnqueued: () => false, + isAgentRunStarted: () => false, + persistUserTurnTranscript: vi.fn(), + session: { agentId: "main", - lifecycleGeneration: "test-generation", + backingSessionId: "sess-main", + cfg: {}, + clientRunId: runId, + now: 1, + rawSessionKey: sessionKey, + sessionKey, }, - activeRunAbort: registration, - cleanupAdmittedRun: registration.cleanup, - lifecycleGeneration: "test-generation", - restartSafeAdmission: {} as never, - }, - context: { - agentRunSeq: new Map(), - broadcast, - chatRunState, - dedupe, - getRuntimeConfig: () => ({}), - logGateway: { warn }, - nodeSendToSession: vi.fn(), - removeChatRun, - } as never, - isQueuedFollowupEnqueued: () => false, - isAgentRunStarted: () => false, - persistUserTurnTranscript: vi.fn(), - session: { - agentId: "main", - backingSessionId: "sess-main", - cfg: {}, - clientRunId: runId, - now: 1, - rawSessionKey: sessionKey, - sessionKey, - }, - terminalizeRestartSafeAdmission, - userTurnRecorder: { hasPersisted: () => true, isBlocked: () => false }, - }); + terminalizeRestartSafeAdmission, + userTurnRecorder: { hasPersisted: () => true, isBlocked: () => false }, + }); - await lifecycle.handleError(new Error("dispatch rejected after explicit abort")); - await lifecycle.finalize(); + if (settlement === "rejects") { + await lifecycle.handleError(new Error("dispatch rejected after explicit abort")); + } + await lifecycle.finalize(); - expect(dedupe.get(`chat:${runId}`)).toMatchObject({ - ok: true, - payload: { runId, status: "timeout", summary: "aborted" }, - }); - expect(broadcast).not.toHaveBeenCalledWith( - "chat", - expect.objectContaining({ runId, state: "error" }), - expect.anything(), - ); - expect(chatAbortControllers.get(runId)).toBe(entry); - expect(entry).toMatchObject({ - projectSessionTerminalPending: true, - registrationCleanupRequested: true, - }); - expect(terminalizeRestartSafeAdmission).not.toHaveBeenCalled(); - } finally { - unsubscribe(); - registration.cleanup(); - } - }); + expect(dedupe.get(`chat:${runId}`)).toMatchObject({ + ok: true, + payload: { runId, status: "timeout", summary: "aborted" }, + }); + expect(broadcast).not.toHaveBeenCalledWith( + "chat", + expect.objectContaining({ runId, state: "error" }), + expect.anything(), + ); + expect(chatAbortControllers.get(runId)).toBe(entry); + expect(entry).toMatchObject({ + projectSessionTerminalPending: true, + registrationCleanupRequested: true, + }); + expect(terminalizeRestartSafeAdmission).not.toHaveBeenCalled(); + } finally { + unsubscribe(); + registration.cleanup(); + } + }, + ); it("keeps a signal-only dispatch rejection as an error without an explicit abort", async () => { const controller = new AbortController(); diff --git a/src/gateway/server-methods/chat-send-dispatch-errors.ts b/src/gateway/server-methods/chat-send-dispatch-errors.ts index a031e025b649..7c555c095c38 100644 --- a/src/gateway/server-methods/chat-send-dispatch-errors.ts +++ b/src/gateway/server-methods/chat-send-dispatch-errors.ts @@ -5,7 +5,7 @@ import { clearAgentRunContext } from "../../infra/agent-run-registry.js"; import type { UserTurnTranscriptRecorder } from "../../sessions/user-turn-transcript.js"; import { captureAgentJobSession, setGatewayDedupeEntry } from "../agent-turn/agent-job.js"; import { ExpectedProfileMismatchError } from "../expected-profile.js"; -import { chatAbortMarkerTimestampMs } from "../server-chat-state.js"; +import { chatAbortMarkerTimestampMs, type ChatAbortMarker } from "../server-chat-state.js"; import { persistGatewaySessionLifecycleEvent } from "../session-lifecycle-state.js"; import { tryResolveSessionCompatibilityOwnerAgentId } from "../session-request-agent.js"; import { formatForLog } from "../ws-log.js"; @@ -120,7 +120,7 @@ export async function handleChatSendSetupError(params: { } } -/** Own dispatch rejection projection and post-cleanup lifecycle persistence. */ +/** Own dispatch settlement and post-cleanup lifecycle persistence. */ export function createChatSendDispatchErrorLifecycle(params: { admission: ChatSendJobAdmission & Pick; context: GatewayRequestContext; @@ -149,32 +149,11 @@ export function createChatSendDispatchErrorLifecycle(params: { admission; const { agentId, backingSessionId, cfg, clientRunId, now, rawSessionKey, sessionKey } = session; const jobSessionBinding = admission.sessionBinding; + let abortedDispatchMarker: ChatAbortMarker | undefined; let pendingDispatchLifecycleError: PendingDispatchLifecycleError | undefined; let persistDispatchErrorUserTurn: (() => Promise) | undefined; let publishDispatchError: (() => void) | undefined; - const recordAbortedResult = () => { - const abortMarker = context.chatRunState.runs.get(clientRunId)?.abortMarker; - if (!activeRunAbort.controller.signal.aborted || abortMarker === undefined) { - return; - } - const endedAt = chatAbortMarkerTimestampMs(abortMarker); - setGatewayDedupeEntry({ - dedupe: context.dedupe, - key: `chat:${clientRunId}`, - session: captureAgentJobSession(jobSessionBinding), - entry: { - ts: endedAt, - ok: true, - payload: buildAbortedChatSendPayload({ - runId: clientRunId, - stopReason: activeRunAbort.entry?.abortStopReason ?? "rpc", - endedAt, - }), - }, - }); - }; - const handleError = async (err: unknown) => { const errorMessage = renderFailoverCodeUserCopy(describeFailoverError(err).code) ?? String(err); const failureDisposition = @@ -202,8 +181,6 @@ export function createChatSendDispatchErrorLifecycle(params: { sessionKey, agentId, }); - } else { - recordAbortedResult(); } return; } @@ -222,7 +199,7 @@ export function createChatSendDispatchErrorLifecycle(params: { // chat.abort has already emitted the canonical terminal lifecycle and // retained its registration until that durable projection settles. // A competing restart-admission write can strand an acknowledged abort. - recordAbortedResult(); + abortedDispatchMarker = abortMarkerAtDispatchReject; context.logGateway.warn( `chat.send post-dispatch threw after abort for runId=${clientRunId}: ${formatForLog(err)}`, ); @@ -338,6 +315,28 @@ export function createChatSendDispatchErrorLifecycle(params: { } }; if (!dispatchError) { + const abortMarker = + abortedDispatchMarker ?? + (activeRunAbort.controller.signal.aborted + ? context.chatRunState.runs.get(clientRunId)?.abortMarker + : undefined); + if (abortMarker) { + const endedAt = chatAbortMarkerTimestampMs(abortMarker); + setGatewayDedupeEntry({ + dedupe: context.dedupe, + key: `chat:${clientRunId}`, + session: captureAgentJobSession(jobSessionBinding), + entry: { + ts: endedAt, + ok: true, + payload: buildAbortedChatSendPayload({ + runId: clientRunId, + stopReason: activeRunAbort.entry?.abortStopReason ?? "rpc", + endedAt, + }), + }, + }); + } clearRun(); cleanupAdmittedRun(); // Reply-dispatch lifecycle events deliberately retain these until delivery settles. @@ -406,5 +405,5 @@ export function createChatSendDispatchErrorLifecycle(params: { } }; - return { finalize, handleError, recordAbortedResult }; + return { finalize, handleError }; } diff --git a/src/gateway/server.cli-watchdog.test.ts b/src/gateway/server.cli-watchdog.test.ts index 733bdbe49aab..79c0877a7a4c 100644 --- a/src/gateway/server.cli-watchdog.test.ts +++ b/src/gateway/server.cli-watchdog.test.ts @@ -13,6 +13,8 @@ import type { import { resolveRuntimeCliBackends } from "../plugins/cli-backends.runtime.js"; import { setTestEnvValue } from "../test-utils/env.js"; import { createOpenClawTestState } from "../test-utils/openclaw-test-state.js"; +import * as agentJobs from "./agent-turn/agent-job.js"; +import type { GatewayClient } from "./client.js"; import * as gatewayFixture from "./test-helpers.e2e.js"; const FREEZE_CONTROLLER = String.raw`const { execFileSync } = require("node:child_process"); @@ -84,6 +86,8 @@ type WatchdogCase = { resume: boolean; }; +type WatchdogCompletion = { status: string; endedAt: number; error?: string }; + const cases: WatchdogCase[] = [ { name: "preserves a fresh CLI reply across a process freeze", @@ -162,6 +166,8 @@ async function runWatchdogCase(testCase: WatchdogCase, signal: AbortSignal) { let orderedOutputAt: number | undefined; let restoreClock: (() => void) | undefined; let observedCredit = false; + const waitForAgentJob = + testCase.behavior === "cancel" ? vi.spyOn(agentJobs, "waitForAgentJob") : undefined; const info = cliBackendLog.info.bind(cliBackendLog); const log = vi.spyOn(cliBackendLog, "info").mockImplementation((...args) => { info(...args); @@ -198,6 +204,7 @@ async function runWatchdogCase(testCase: WatchdogCase, signal: AbortSignal) { }); const proof = state.path("proof"); let gateway: Awaited> | undefined; + let pendingCompletion: Promise | undefined; try { await fs.mkdir(proof); await fs.writeFile(path.join(proof, "freeze-tree.cjs"), FREEZE_CONTROLLER); @@ -377,7 +384,27 @@ createInterface({ input: process.stdin }).on("line", (line) => { await fs.readFile(path.join(nativeRoot, "ready.json"), "utf8"), ); expect(ready.turns).toBe(testCase.resume ? 2 : 1); + const waitForCompletion = (client: GatewayClient) => + client.request( + "agent.wait", + { + runId: accepted.runId, + timeoutMs: 50_000, + }, + { timeoutMs: 55_000 }, + ); if (testCase.behavior === "cancel") { + pendingCompletion = waitForCompletion(gateway.client); + void pendingCompletion.catch(() => {}); + await expect + .poll( + () => + waitForAgentJob?.mock.calls.some( + ([params]) => params.runId === accepted.runId && params.source === "chat", + ), + { timeout: 5_000 }, + ) + .toBe(true); const cancelled = await gateway.client.request("chat.abort", { sessionKey, runId: accepted.runId, @@ -421,18 +448,7 @@ createInterface({ input: process.stdin }).on("line", (line) => { scopes: ["operator.admin", "operator.read", "operator.write"], }); } - const completed = await gateway.client.request<{ - status: string; - endedAt: number; - error?: string; - }>( - "agent.wait", - { - runId: accepted.runId, - timeoutMs: 50_000, - }, - { timeoutMs: 55_000 }, - ); + const completed = await (pendingCompletion ?? waitForCompletion(gateway.client)); const history = await gateway.client.request("chat.history", { sessionKey }); await fs.writeFile( path.join(proof, "gateway-result.json"), @@ -476,6 +492,7 @@ createInterface({ input: process.stdin }).on("line", (line) => { } finally { restoreClock?.(); log.mockRestore(); + waitForAgentJob?.mockRestore(); try { const evidenceRoot = process.env.OPENCLAW_CLI_WATCHDOG_PROOF_DIR; if (evidenceRoot) { @@ -485,6 +502,7 @@ createInterface({ input: process.stdin }).on("line", (line) => { } finally { testing.resetDepsForTest(); gateway?.client.stop(); + await pendingCompletion?.catch(() => {}); try { await gateway?.server.close({ reason: "freeze proof complete" }); } finally { diff --git a/src/plugin-sdk/process-runtime.ts b/src/plugin-sdk/process-runtime.ts index a320e0ac7b12..fb2be4b21187 100644 --- a/src/plugin-sdk/process-runtime.ts +++ b/src/plugin-sdk/process-runtime.ts @@ -20,6 +20,12 @@ export { resolveRuntimeWorkerArgv, resolveRuntimeWorkerUrl } from "../infra/runt export { WorkerTaskError, WorkerTaskPool, serveWorkerTasks } from "../infra/worker-task-pool.js"; export type { WorkerTaskControl } from "../infra/worker-task-pool.js"; export { killProcessTree, signalProcessTree } from "../process/kill-tree.js"; +export { + spawnTerminalPty, + type TerminalPtyHandle, + type TerminalPtySpawnParams, + type TerminalPtySubscription, +} from "../process/terminal-pty.js"; export { getFileLockProcessStartTime, isPidAlive, diff --git a/src/plugin-sdk/sandbox.ts b/src/plugin-sdk/sandbox.ts index 962de84718f0..e7bf4abe7efa 100644 --- a/src/plugin-sdk/sandbox.ts +++ b/src/plugin-sdk/sandbox.ts @@ -31,6 +31,7 @@ export type { export type { OpenClawConfig } from "../config/config.js"; export type { DirectoryEntry } from "../infra/directory-entries.js"; export { resolveReadOnlyWorkspaceSkillMounts } from "../agents/sandbox/workspace-mounts.js"; +export { SANDBOX_COMMAND_MAX_BUFFER_BYTES } from "../agents/sandbox/constants.js"; export { buildExecRemoteCommand, diff --git a/src/process/terminal-pty-node.failure.test.ts b/src/process/terminal-pty-node.failure.test.ts index d37150d71f3f..f1d264db70d8 100644 --- a/src/process/terminal-pty-node.failure.test.ts +++ b/src/process/terminal-pty-node.failure.test.ts @@ -1,6 +1,11 @@ +import type { ChildProcess } from "node:child_process"; import { EventEmitter } from "node:events"; +import os from "node:os"; +import path from "node:path"; import { PassThrough } from "node:stream"; import { afterEach, expect, it, vi } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; +import { createDeferredCore } from "../shared/deferred.js"; import { withMockedPlatform } from "../test-utils/vitest-spies.js"; import { spawnNodeTerminalPty } from "./terminal-pty-node.js"; @@ -25,15 +30,14 @@ vi.mock("node:fs", async (importOriginal) => { vi.mock("../infra/node-runtime-executable.js", () => ({ resolveNodeRuntimeExecutable: () => process.execPath, })); -vi.mock("../infra/runtime-worker-url.js", () => ({ - resolveRuntimeWorkerUrl: () => new URL("file:///fixture/terminal-worker.js"), - resolveRuntimeWorkerArgv: () => [], -})); + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); afterEach(() => { vi.restoreAllMocks(); vi.clearAllTimers(); vi.useRealTimers(); + spawnMock.mockReset(); }); it("joins failed terminal startup cleanup when procfs identity is unavailable", async () => { @@ -81,3 +85,56 @@ it("joins failed terminal startup cleanup when procfs identity is unavailable", expect(signals.mock.calls).toEqual([[7777, "SIGKILL"]]); }); }); + +it.runIf(process.platform !== "win32")( + "joins the real helper before releasing cancelled PTY startup", + async () => { + const actual = await vi.importActual("node:child_process"); + const helperExit = createDeferredCore(); + let helper: ChildProcess | undefined; + let helperExited = false; + spawnMock.mockImplementation((...args: Parameters) => { + helper = actual.spawn(...args); + helper.once("exit", () => { + helperExited = true; + helperExit.resolve(); + }); + return helper; + }); + try { + await expect( + spawnNodeTerminalPty( + { file: "/bin/sh", args: [], cwd: os.tmpdir(), cols: 80, rows: 24 }, + () => { + throw new Error("PTY policy revoked"); + }, + ), + ).rejects.toThrow("PTY policy revoked"); + expect(helperExited).toBe(true); + expect(helper?.stdout?.readableEnded || helper?.stdout?.destroyed).toBe(true); + expect(helper?.connected).toBe(false); + } finally { + if (helper && !helperExited) { + helper.kill("SIGKILL"); + await helperExit.promise; + } + } + }, +); + +it("settles a real helper spawn failure without requiring an exit event", async () => { + const actual = await vi.importActual("node:child_process"); + const missingNode = path.join(tempDirs.make("openclaw-pty-missing-node-"), "missing-node"); + let helper: ChildProcess | undefined; + spawnMock.mockImplementation( + (_file: string, args: string[], options: Parameters[2]) => { + helper = actual.spawn(missingNode, args, options); + return helper; + }, + ); + await expect( + spawnNodeTerminalPty({ file: "/bin/sh", args: [], cols: 80, rows: 24 }), + ).rejects.toMatchObject({ code: "ENOENT" }); + expect(helper?.pid).toBeUndefined(); + expect(helper?.stdout?.readableEnded || helper?.stdout?.destroyed).toBe(true); +});