From 1cbd5a94703ff79d36bd7ac0aee5e0aef4662317 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 23 Apr 2026 02:14:37 +0100 Subject: [PATCH] fix(codex): harden app-server approvals --- .../src/app-server/approval-bridge.test.ts | 19 + .../codex/src/app-server/approval-bridge.ts | 139 ++----- .../codex/src/app-server/client.test.ts | 23 +- .../src/app-server/elicitation-bridge.test.ts | 284 ++++++++++++++ .../src/app-server/elicitation-bridge.ts | 345 ++++++++++++++++++ .../src/app-server/event-projector.test.ts | 19 + .../codex/src/app-server/event-projector.ts | 2 +- .../app-server/plugin-approval-roundtrip.ts | 106 ++++++ .../codex/src/app-server/run-attempt.test.ts | 132 ++++++- .../codex/src/app-server/run-attempt.ts | 26 +- src/auto-reply/get-reply-options.types.ts | 1 + .../reply/agent-runner-execution.test.ts | 1 + .../reply/agent-runner-execution.ts | 5 + src/infra/agent-events.ts | 1 + 14 files changed, 985 insertions(+), 118 deletions(-) create mode 100644 extensions/codex/src/app-server/elicitation-bridge.test.ts create mode 100644 extensions/codex/src/app-server/elicitation-bridge.ts create mode 100644 extensions/codex/src/app-server/plugin-approval-roundtrip.ts diff --git a/extensions/codex/src/app-server/approval-bridge.test.ts b/extensions/codex/src/app-server/approval-bridge.test.ts index 282d49b24e2..2873a039940 100644 --- a/extensions/codex/src/app-server/approval-bridge.test.ts +++ b/extensions/codex/src/app-server/approval-bridge.test.ts @@ -234,6 +234,25 @@ describe("Codex app-server approval bridge", () => { expect(description).toContain("High-risk targets:"); }); + it("ignores approval requests that are missing explicit thread or turn ids", async () => { + const params = createParams(); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + itemId: "cmd-2", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toBeUndefined(); + expect(mockCallGatewayTool).not.toHaveBeenCalled(); + expect(params.onAgentEvent).not.toHaveBeenCalled(); + }); + it("maps app-server approval response families separately", () => { expect( buildApprovalResponse( diff --git a/extensions/codex/src/app-server/approval-bridge.ts b/extensions/codex/src/app-server/approval-bridge.ts index 7c7509a2854..cde81e39784 100644 --- a/extensions/codex/src/app-server/approval-bridge.ts +++ b/extensions/codex/src/app-server/approval-bridge.ts @@ -1,34 +1,18 @@ import { - callGatewayTool, type AgentApprovalEventData, type EmbeddedRunAttemptParams, - type ExecApprovalDecision, } from "openclaw/plugin-sdk/agent-harness"; +import { + mapExecDecisionToOutcome, + requestPluginApproval, + type AppServerApprovalOutcome, + waitForPluginApprovalDecision, +} from "./plugin-approval-roundtrip.js"; import { isJsonObject, type JsonObject, type JsonValue } from "./protocol.js"; -const DEFAULT_CODEX_APPROVAL_TIMEOUT_MS = 120_000; const PERMISSION_DESCRIPTION_MAX_LENGTH = 700; const PERMISSION_SAMPLE_LIMIT = 2; const PERMISSION_VALUE_MAX_LENGTH = 48; - -export type AppServerApprovalOutcome = - | "approved-once" - | "approved-session" - | "denied" - | "unavailable" - | "cancelled"; - -type ApprovalRequestResult = { - id?: string; - status?: string; - decision?: ExecApprovalDecision | null; -}; - -type ApprovalWaitResult = { - id?: string; - decision?: ExecApprovalDecision | null; -}; - export async function handleCodexAppServerApprovalRequest(params: { method: string; requestParams: JsonValue | undefined; @@ -52,29 +36,14 @@ export async function handleCodexAppServerApprovalRequest(params: { }); try { - const timeoutMs = DEFAULT_CODEX_APPROVAL_TIMEOUT_MS; - const requestResult: ApprovalRequestResult | undefined = await callGatewayTool( - "plugin.approval.request", - { timeoutMs: timeoutMs + 10_000 }, - { - pluginId: "openclaw-codex-app-server", - title: context.title, - description: context.description, - severity: context.severity, - toolName: context.toolName, - toolCallId: context.itemId, - agentId: params.paramsForRun.agentId, - sessionKey: params.paramsForRun.sessionKey, - turnSourceChannel: - params.paramsForRun.messageChannel ?? params.paramsForRun.messageProvider, - turnSourceTo: params.paramsForRun.currentChannelId, - turnSourceAccountId: params.paramsForRun.agentAccountId, - turnSourceThreadId: params.paramsForRun.currentThreadTs, - timeoutMs, - twoPhase: true, - }, - { expectFinal: false }, - ); + const requestResult = await requestPluginApproval({ + paramsForRun: params.paramsForRun, + title: context.title, + description: context.description, + severity: context.severity, + toolName: context.toolName, + toolCallId: context.itemId, + }); const approvalId = requestResult?.id; if (!approvalId) { @@ -84,6 +53,7 @@ export async function handleCodexAppServerApprovalRequest(params: { status: "unavailable", title: context.title, ...context.eventDetails, + ...approvalEventScope(params.method, "denied"), message: "Codex app-server approval route unavailable.", }); return buildApprovalResponse(params.method, context.requestParams, "denied"); @@ -102,11 +72,7 @@ export async function handleCodexAppServerApprovalRequest(params: { const decision = Object.prototype.hasOwnProperty.call(requestResult, "decision") ? requestResult.decision - : await waitForApprovalDecision({ - approvalId, - timeoutMs, - signal: params.signal, - }); + : await waitForPluginApprovalDecision({ approvalId, signal: params.signal }); const outcome = mapExecDecisionToOutcome(decision); emitApprovalEvent(params.paramsForRun, { @@ -124,6 +90,7 @@ export async function handleCodexAppServerApprovalRequest(params: { approvalId, approvalSlug: approvalId, ...context.eventDetails, + ...approvalEventScope(params.method, outcome), message: approvalResolutionMessage(outcome), }); return buildApprovalResponse(params.method, context.requestParams, outcome); @@ -135,6 +102,7 @@ export async function handleCodexAppServerApprovalRequest(params: { status: cancelled ? "failed" : "unavailable", title: context.title, ...context.eventDetails, + ...approvalEventScope(params.method, cancelled ? "cancelled" : "denied"), message: cancelled ? "Codex app-server approval cancelled because the run stopped." : `Codex app-server approval route failed: ${formatErrorMessage(error)}`, @@ -176,18 +144,12 @@ function matchesCurrentTurn( turnId: string, ): boolean { if (!requestParams) { - return true; + return false; } const requestThreadId = readString(requestParams, "threadId") ?? readString(requestParams, "conversationId"); const requestTurnId = readString(requestParams, "turnId"); - if (requestThreadId && requestThreadId !== threadId) { - return false; - } - if (requestTurnId && requestTurnId !== turnId) { - return false; - } - return true; + return requestThreadId === threadId && requestTurnId === turnId; } function buildApprovalContext(params: { @@ -248,37 +210,6 @@ function buildApprovalContext(params: { }; } -async function waitForApprovalDecision(params: { - approvalId: string; - timeoutMs: number; - signal?: AbortSignal; -}): Promise { - const waitPromise: Promise = callGatewayTool( - "plugin.approval.waitDecision", - { timeoutMs: params.timeoutMs + 10_000 }, - { id: params.approvalId }, - ); - if (!params.signal) { - return (await waitPromise)?.decision; - } - let onAbort: (() => void) | undefined; - const abortPromise = new Promise((_, reject) => { - if (params.signal!.aborted) { - reject(params.signal!.reason); - return; - } - onAbort = () => reject(params.signal!.reason); - params.signal!.addEventListener("abort", onAbort, { once: true }); - }); - try { - return (await Promise.race([waitPromise, abortPromise]))?.decision; - } finally { - if (onAbort) { - params.signal.removeEventListener("abort", onAbort); - } - } -} - function commandApprovalDecision( requestParams: JsonObject | undefined, outcome: AppServerApprovalOutcome, @@ -528,27 +459,12 @@ function hasAvailableDecision(requestParams: JsonObject | undefined, decision: s return available.includes(decision); } -function mapExecDecisionToOutcome( - decision: ExecApprovalDecision | null | undefined, -): AppServerApprovalOutcome { - if (decision === "allow-once") { - return "approved-once"; - } - if (decision === "allow-always") { - return "approved-session"; - } - if (decision === null || decision === undefined) { - return "unavailable"; - } - return "denied"; -} - function approvalResolutionMessage(outcome: AppServerApprovalOutcome): string { if (outcome === "approved-session") { return "Codex app-server approval granted for the session."; } if (outcome === "approved-once") { - return "Codex app-server approval granted once."; + return "Codex app-server approval granted for this turn."; } if (outcome === "cancelled") { return "Codex app-server approval cancelled."; @@ -559,6 +475,19 @@ function approvalResolutionMessage(outcome: AppServerApprovalOutcome): string { return "Codex app-server approval denied."; } +function approvalScopeForOutcome(outcome: AppServerApprovalOutcome): "turn" | "session" { + return outcome === "approved-session" ? "session" : "turn"; +} + +function approvalEventScope( + method: string, + outcome: AppServerApprovalOutcome, +): Pick { + return method === "item/permissions/requestApproval" + ? { scope: approvalScopeForOutcome(outcome) } + : {}; +} + function approvalKindForMethod(method: string): AgentApprovalEventData["kind"] { if (method.includes("commandExecution") || method.includes("execCommand")) { return "exec"; diff --git a/extensions/codex/src/app-server/client.test.ts b/extensions/codex/src/app-server/client.test.ts index 238a51efdde..8db9e440430 100644 --- a/extensions/codex/src/app-server/client.test.ts +++ b/extensions/codex/src/app-server/client.test.ts @@ -31,7 +31,6 @@ describe("CodexAppServerClient", () => { resetSharedCodexAppServerClientForTests(); vi.useRealTimers(); vi.restoreAllMocks(); - vi.useRealTimers(); for (const client of clients) { client.close(); } @@ -253,4 +252,26 @@ describe("CodexAppServerClient", () => { expect(isCodexAppServerApprovalRequest("evil/Approval")).toBe(false); expect(isCodexAppServerApprovalRequest("item/tool/requestApproval")).toBe(false); }); + + it("fails closed for unhandled request_user_input prompts", async () => { + const harness = createClientHarness(); + clients.push(harness.client); + + harness.send({ + id: "input-1", + method: "item/tool/requestUserInput", + params: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "tool-1", + questions: [], + }, + }); + await vi.waitFor(() => expect(harness.writes.length).toBe(1)); + + expect(JSON.parse(harness.writes[0] ?? "{}")).toEqual({ + id: "input-1", + result: { answers: {} }, + }); + }); }); diff --git a/extensions/codex/src/app-server/elicitation-bridge.test.ts b/extensions/codex/src/app-server/elicitation-bridge.test.ts new file mode 100644 index 00000000000..8cafa1347ba --- /dev/null +++ b/extensions/codex/src/app-server/elicitation-bridge.test.ts @@ -0,0 +1,284 @@ +import { + callGatewayTool, + embeddedAgentLog, + type EmbeddedRunAttemptParams, +} from "openclaw/plugin-sdk/agent-harness"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { handleCodexAppServerElicitationRequest } from "./elicitation-bridge.js"; + +vi.mock("openclaw/plugin-sdk/agent-harness", async (importOriginal) => ({ + ...(await importOriginal()), + callGatewayTool: vi.fn(), +})); + +const mockCallGatewayTool = vi.mocked(callGatewayTool); + +function createParams(): EmbeddedRunAttemptParams { + return { + sessionKey: "agent:main:session-1", + agentId: "main", + messageChannel: "telegram", + currentChannelId: "chat-1", + agentAccountId: "default", + currentThreadTs: "thread-ts", + } as unknown as EmbeddedRunAttemptParams; +} + +function buildApprovalElicitation() { + return { + threadId: "thread-1", + turnId: "turn-1", + serverName: "codex_apps__github", + mode: "form", + message: "Approve app tool call?", + _meta: { + codex_approval_kind: "mcp_tool_call", + persist: ["session", "always"], + }, + requestedSchema: { + type: "object", + properties: { + approve: { + type: "boolean", + title: "Approve this tool call", + }, + persist: { + type: "string", + title: "Persist choice", + enum: ["session", "always"], + }, + }, + required: ["approve"], + }, + }; +} + +describe("Codex app-server elicitation bridge", () => { + beforeEach(() => { + mockCallGatewayTool.mockReset(); + vi.restoreAllMocks(); + }); + + it("routes MCP tool approval elicitations through plugin approvals", async () => { + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-1", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-1", decision: "allow-once" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: buildApprovalElicitation(), + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "accept", + content: { + approve: true, + }, + _meta: null, + }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + }); + + it("maps allow-always decisions onto session-scoped persistence when offered", async () => { + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-2", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-2", decision: "allow-always" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: buildApprovalElicitation(), + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "accept", + content: { + approve: true, + persist: "session", + }, + _meta: null, + }); + }); + + it("does not inherit persist defaults for one-time approvals", async () => { + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-5", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-5", decision: "allow-once" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: { + ...buildApprovalElicitation(), + requestedSchema: { + type: "object", + properties: { + approve: { + type: "boolean", + title: "Approve this tool call", + }, + persist: { + type: "string", + title: "Persist choice", + enum: ["session", "always"], + default: "always", + }, + }, + required: ["approve"], + }, + }, + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "accept", + content: { + approve: true, + }, + _meta: null, + }); + }); + + it("truncates long approval titles and descriptions before requesting approval", async () => { + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-4", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-4", decision: "allow-once" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: { + ...buildApprovalElicitation(), + message: "Approve ".repeat(20).trim(), + requestedSchema: { + type: "object", + properties: { + approve: { + type: "boolean", + title: "Approve this tool call", + description: "Explain ".repeat(60).trim(), + }, + }, + required: ["approve"], + }, + }, + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "accept", + content: { + approve: true, + }, + _meta: null, + }); + expect(mockCallGatewayTool).toHaveBeenCalledWith( + "plugin.approval.request", + expect.any(Object), + expect.objectContaining({ + title: expect.any(String), + description: expect.any(String), + }), + { expectFinal: false }, + ); + const approvalRequest = mockCallGatewayTool.mock.calls[0]?.[2] as { + title: string; + description: string; + }; + expect(approvalRequest.title.length).toBeLessThanOrEqual(80); + expect(approvalRequest.description.length).toBeLessThanOrEqual(256); + }); + + it("fails closed when the approval route is unavailable", async () => { + mockCallGatewayTool.mockResolvedValueOnce({ id: "plugin:approval-3", decision: null }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: buildApprovalElicitation(), + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "decline", + content: null, + _meta: null, + }); + }); + + it("ignores non-approval elicitation requests", async () => { + const result = await handleCodexAppServerElicitationRequest({ + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + serverName: "codex_apps__github", + mode: "form", + message: "Choose a template", + _meta: {}, + requestedSchema: { + type: "object", + properties: { + template: { + type: "string", + enum: ["simple", "fancy"], + }, + }, + required: ["template"], + }, + }, + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toBeUndefined(); + expect(mockCallGatewayTool).not.toHaveBeenCalled(); + }); + + it("logs and declines approved elicitations that do not expose an approval field", async () => { + const warn = vi.spyOn(embeddedAgentLog, "warn").mockImplementation(() => undefined); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-6", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-6", decision: "allow-once" }); + + const result = await handleCodexAppServerElicitationRequest({ + requestParams: { + ...buildApprovalElicitation(), + requestedSchema: { + type: "object", + properties: { + confirmChoice: { + type: "string", + title: "Confirmation choice", + enum: ["yes", "no"], + }, + }, + required: ["confirmChoice"], + }, + }, + paramsForRun: createParams(), + threadId: "thread-1", + turnId: "turn-1", + }); + + expect(result).toEqual({ + action: "decline", + content: null, + _meta: null, + }); + expect(warn).toHaveBeenCalledWith( + "codex MCP approval elicitation approved without a mappable response", + expect.objectContaining({ + approvalKind: "mcp_tool_call", + fields: ["confirmChoice"], + outcome: "approved-once", + }), + ); + }); +}); diff --git a/extensions/codex/src/app-server/elicitation-bridge.ts b/extensions/codex/src/app-server/elicitation-bridge.ts new file mode 100644 index 00000000000..714d88a6d5b --- /dev/null +++ b/extensions/codex/src/app-server/elicitation-bridge.ts @@ -0,0 +1,345 @@ +import { embeddedAgentLog, type EmbeddedRunAttemptParams } from "openclaw/plugin-sdk/agent-harness"; +import { + mapExecDecisionToOutcome, + requestPluginApproval, + type AppServerApprovalOutcome, + waitForPluginApprovalDecision, +} from "./plugin-approval-roundtrip.js"; +import { isJsonObject, type JsonObject, type JsonValue } from "./protocol.js"; + +type ApprovalPropertyContext = { + name: string; + schema: JsonObject; + required: boolean; +}; + +type BridgeableApprovalElicitation = { + title: string; + description: string; + requestedSchema: JsonObject; + meta: JsonObject; +}; + +export async function handleCodexAppServerElicitationRequest(params: { + requestParams: JsonValue | undefined; + paramsForRun: EmbeddedRunAttemptParams; + threadId: string; + turnId: string; + signal?: AbortSignal; +}): Promise { + const requestParams = isJsonObject(params.requestParams) ? params.requestParams : undefined; + if (!matchesCurrentTurn(requestParams, params.threadId, params.turnId)) { + return undefined; + } + const approvalPrompt = readBridgeableApprovalElicitation(requestParams); + if (!approvalPrompt) { + return undefined; + } + + const outcome = await requestPluginApprovalOutcome({ + paramsForRun: params.paramsForRun, + title: approvalPrompt.title, + description: approvalPrompt.description, + signal: params.signal, + }); + return buildElicitationResponse(approvalPrompt.requestedSchema, approvalPrompt.meta, outcome); +} + +function matchesCurrentTurn( + requestParams: JsonObject | undefined, + threadId: string, + turnId: string, +): boolean { + if (!requestParams) { + return false; + } + const requestThreadId = readString(requestParams, "threadId"); + if (requestThreadId !== threadId) { + return false; + } + const rawTurnId = requestParams.turnId; + if (rawTurnId !== null && rawTurnId !== undefined && rawTurnId !== turnId) { + return false; + } + return true; +} + +function readBridgeableApprovalElicitation( + requestParams: JsonObject | undefined, +): BridgeableApprovalElicitation | undefined { + if ( + !requestParams || + readString(requestParams, "mode") !== "form" || + !isJsonObject(requestParams._meta) || + requestParams._meta.codex_approval_kind !== "mcp_tool_call" || + !isJsonObject(requestParams.requestedSchema) + ) { + return undefined; + } + + const requestedSchema = requestParams.requestedSchema; + if ( + readString(requestedSchema, "type") !== "object" || + !isJsonObject(requestedSchema.properties) || + Object.keys(requestedSchema.properties).length === 0 + ) { + return undefined; + } + + const title = readString(requestParams, "message") ?? "Codex MCP tool approval"; + const propertyLines = Object.entries(requestedSchema.properties) + .map(([name, value]) => { + const schema = isJsonObject(value) ? value : undefined; + if (!schema) { + return undefined; + } + const propTitle = readString(schema, "title") ?? name; + const description = readString(schema, "description"); + return description ? `- ${propTitle}: ${description}` : `- ${propTitle}`; + }) + .filter((line): line is string => Boolean(line)); + + return { + title, + description: [title, propertyLines.length > 0 ? ["Fields:", ...propertyLines].join("\n") : ""] + .filter(Boolean) + .join("\n\n"), + requestedSchema, + meta: requestParams._meta, + }; +} + +async function requestPluginApprovalOutcome(params: { + paramsForRun: EmbeddedRunAttemptParams; + title: string; + description: string; + signal?: AbortSignal; +}): Promise { + try { + const requestResult = await requestPluginApproval({ + paramsForRun: params.paramsForRun, + title: params.title, + description: params.description, + severity: "warning", + toolName: "codex_mcp_tool_approval", + }); + + const approvalId = requestResult?.id; + if (!approvalId) { + return "unavailable"; + } + + const decision = Object.prototype.hasOwnProperty.call(requestResult, "decision") + ? requestResult.decision + : await waitForPluginApprovalDecision({ approvalId, signal: params.signal }); + return mapExecDecisionToOutcome(decision); + } catch { + return params.signal?.aborted ? "cancelled" : "denied"; + } +} + +function buildElicitationResponse( + requestedSchema: JsonObject, + meta: JsonObject, + outcome: AppServerApprovalOutcome, +): JsonValue { + if (outcome === "cancelled") { + return { action: "cancel", content: null, _meta: null }; + } + if (outcome === "denied" || outcome === "unavailable") { + return { action: "decline", content: null, _meta: null }; + } + + const content = buildAcceptedContent(requestedSchema, meta, outcome); + if (!content) { + embeddedAgentLog.warn("codex MCP approval elicitation approved without a mappable response", { + approvalKind: meta.codex_approval_kind, + fields: Object.keys(requestedSchema.properties ?? {}), + outcome, + }); + return { action: "decline", content: null, _meta: null }; + } + return { action: "accept", content, _meta: null }; +} + +function buildAcceptedContent( + requestedSchema: JsonObject, + meta: JsonObject, + outcome: AppServerApprovalOutcome, +): JsonObject | undefined { + const properties = isJsonObject(requestedSchema.properties) + ? requestedSchema.properties + : undefined; + if (!properties) { + return undefined; + } + const required = Array.isArray(requestedSchema.required) + ? new Set( + requestedSchema.required.filter((entry): entry is string => typeof entry === "string"), + ) + : new Set(); + const content: JsonObject = {}; + let sawApprovalField = false; + + for (const [name, value] of Object.entries(properties)) { + const schema = isJsonObject(value) ? value : undefined; + if (!schema) { + continue; + } + const property = { name, schema, required: required.has(name) }; + const next = + readApprovalFieldValue(property, outcome) ?? + readPersistFieldValue(property, meta, outcome) ?? + readFallbackFieldValue(property, outcome); + + if (next === undefined) { + if (isApprovalField(property)) { + sawApprovalField = true; + } + if (property.required) { + return undefined; + } + continue; + } + + if (isApprovalField(property)) { + sawApprovalField = true; + } + content[name] = next; + } + + return sawApprovalField ? content : undefined; +} + +function readApprovalFieldValue( + property: ApprovalPropertyContext, + outcome: AppServerApprovalOutcome, +): JsonValue | undefined { + if (!isApprovalField(property)) { + return undefined; + } + const type = readString(property.schema, "type"); + if (type === "boolean") { + return true; + } + const options = readEnumOptions(property.schema); + if (options.length === 0) { + return undefined; + } + + const sessionChoice = options.find((option) => isSessionApprovalOption(option)); + const acceptChoice = options.find((option) => isPositiveApprovalOption(option)); + if (outcome === "approved-session") { + return sessionChoice?.value ?? acceptChoice?.value; + } + return acceptChoice?.value ?? sessionChoice?.value; +} + +function readPersistFieldValue( + property: ApprovalPropertyContext, + meta: JsonObject, + outcome: AppServerApprovalOutcome, +): JsonValue | undefined { + if (!isPersistField(property) || outcome !== "approved-session") { + return undefined; + } + const persistHints = readPersistHints(meta); + const options = readEnumOptions(property.schema); + if (options.length === 0) { + return undefined; + } + for (const preferred of persistHints) { + const match = options.find( + (option) => option.value === preferred || option.label === preferred, + ); + if (match) { + return match.value; + } + } + return options.find((option) => option.value === "session" || option.label === "session")?.value; +} + +function readDefaultValue(schema: JsonObject): JsonValue | undefined { + return schema.default as JsonValue | undefined; +} + +function readFallbackFieldValue( + property: ApprovalPropertyContext, + outcome: AppServerApprovalOutcome, +): JsonValue | undefined { + if (outcome === "approved-once" && isPersistField(property)) { + return undefined; + } + return readDefaultValue(property.schema); +} + +function isApprovalField(property: ApprovalPropertyContext): boolean { + const haystack = propertyText(property).toLowerCase(); + return /\b(approve|approval|allow|accept|decision)\b/.test(haystack); +} + +function isPersistField(property: ApprovalPropertyContext): boolean { + const haystack = propertyText(property).toLowerCase(); + return /\b(persist|session|always|scope)\b/.test(haystack); +} + +function propertyText(property: ApprovalPropertyContext): string { + return [ + property.name, + readString(property.schema, "title"), + readString(property.schema, "description"), + ] + .filter(Boolean) + .join(" "); +} + +function readPersistHints(meta: JsonObject): string[] { + const raw = meta.persist; + if (typeof raw === "string") { + return [raw]; + } + if (Array.isArray(raw)) { + return raw.filter((entry): entry is string => typeof entry === "string"); + } + return ["session", "always"]; +} + +function readEnumOptions(schema: JsonObject): Array<{ value: string; label: string }> { + if (Array.isArray(schema.enum)) { + const values = schema.enum.filter((entry): entry is string => typeof entry === "string"); + const labels = Array.isArray(schema.enumNames) + ? schema.enumNames.filter((entry): entry is string => typeof entry === "string") + : []; + return values.map((value, index) => ({ value, label: labels[index] ?? value })); + } + if (Array.isArray(schema.oneOf)) { + return schema.oneOf + .map((entry) => { + const option = isJsonObject(entry) ? entry : undefined; + const value = readString(option, "const"); + if (!value) { + return undefined; + } + return { value, label: readString(option, "title") ?? value }; + }) + .filter((entry): entry is { value: string; label: string } => Boolean(entry)); + } + return []; +} + +function isPositiveApprovalOption(option: { value: string; label: string }): boolean { + const haystack = `${option.value} ${option.label}`.toLowerCase(); + return /\b(allow|approve|accept|yes|continue|proceed|true)\b/.test(haystack); +} + +function isSessionApprovalOption(option: { value: string; label: string }): boolean { + const haystack = `${option.value} ${option.label}`.toLowerCase(); + return ( + /\b(session|always|persistent)\b/.test(haystack) && /\b(allow|approve|accept)\b/.test(haystack) + ); +} + +function readString(record: JsonObject | undefined, key: string): string | undefined { + const value = record?.[key]; + return typeof value === "string" && value.trim() ? value : undefined; +} diff --git a/extensions/codex/src/app-server/event-projector.test.ts b/extensions/codex/src/app-server/event-projector.test.ts index 6a834ab86c9..7fc5eb203e2 100644 --- a/extensions/codex/src/app-server/event-projector.test.ts +++ b/extensions/codex/src/app-server/event-projector.test.ts @@ -292,6 +292,25 @@ describe("CodexAppServerEventProjector", () => { expect(result.assistantTexts).toEqual([]); }); + it("ignores notifications that omit top-level thread and turn ids", async () => { + const projector = await createProjector(); + + await projector.handleNotification({ + method: "turn/completed", + params: { + turn: { + id: TURN_ID, + status: "completed", + items: [{ type: "agentMessage", id: "msg-1", text: "wrong turn" }], + }, + }, + }); + + const result = projector.buildResult(buildEmptyToolTelemetry()); + expect(result.assistantTexts).toEqual([]); + expect(result.lastAssistant).toBeUndefined(); + }); + it("preserves sessions_yield detection in attempt results", () => { const projector = new CodexAppServerEventProjector( { diff --git a/extensions/codex/src/app-server/event-projector.ts b/extensions/codex/src/app-server/event-projector.ts index a68d60d9f54..687ad351b93 100644 --- a/extensions/codex/src/app-server/event-projector.ts +++ b/extensions/codex/src/app-server/event-projector.ts @@ -577,7 +577,7 @@ export class CodexAppServerEventProjector { private isNotificationForTurn(params: JsonObject): boolean { const threadId = readString(params, "threadId"); const turnId = readString(params, "turnId"); - return (!threadId || threadId === this.threadId) && (!turnId || turnId === this.turnId); + return threadId === this.threadId && turnId === this.turnId; } } diff --git a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts new file mode 100644 index 00000000000..98e9b1b6445 --- /dev/null +++ b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts @@ -0,0 +1,106 @@ +import { callGatewayTool, type EmbeddedRunAttemptParams } from "openclaw/plugin-sdk/agent-harness"; + +export const DEFAULT_CODEX_APPROVAL_TIMEOUT_MS = 120_000; +const MAX_PLUGIN_APPROVAL_TITLE_LENGTH = 80; +const MAX_PLUGIN_APPROVAL_DESCRIPTION_LENGTH = 256; + +type ExecApprovalDecision = "allow-once" | "allow-always" | "deny"; + +export type AppServerApprovalOutcome = + | "approved-once" + | "approved-session" + | "denied" + | "unavailable" + | "cancelled"; + +type ApprovalRequestResult = { + id?: string; + decision?: ExecApprovalDecision | null; +}; + +type ApprovalWaitResult = { + id?: string; + decision?: ExecApprovalDecision | null; +}; + +export async function requestPluginApproval(params: { + paramsForRun: EmbeddedRunAttemptParams; + title: string; + description: string; + severity: "info" | "warning"; + toolName: string; + toolCallId?: string; +}): Promise { + const timeoutMs = DEFAULT_CODEX_APPROVAL_TIMEOUT_MS; + return callGatewayTool( + "plugin.approval.request", + { timeoutMs: timeoutMs + 10_000 }, + { + pluginId: "openclaw-codex-app-server", + title: truncateForGateway(params.title, MAX_PLUGIN_APPROVAL_TITLE_LENGTH), + description: truncateForGateway(params.description, MAX_PLUGIN_APPROVAL_DESCRIPTION_LENGTH), + severity: params.severity, + toolName: params.toolName, + toolCallId: params.toolCallId, + agentId: params.paramsForRun.agentId, + sessionKey: params.paramsForRun.sessionKey, + turnSourceChannel: params.paramsForRun.messageChannel ?? params.paramsForRun.messageProvider, + turnSourceTo: params.paramsForRun.currentChannelId, + turnSourceAccountId: params.paramsForRun.agentAccountId, + turnSourceThreadId: params.paramsForRun.currentThreadTs, + timeoutMs, + twoPhase: true, + }, + { expectFinal: false }, + ) as Promise; +} + +export async function waitForPluginApprovalDecision(params: { + approvalId: string; + signal?: AbortSignal; +}): Promise { + const timeoutMs = DEFAULT_CODEX_APPROVAL_TIMEOUT_MS; + const waitPromise: Promise = callGatewayTool( + "plugin.approval.waitDecision", + { timeoutMs: timeoutMs + 10_000 }, + { id: params.approvalId }, + ); + if (!params.signal) { + return (await waitPromise)?.decision; + } + let onAbort: (() => void) | undefined; + const abortPromise = new Promise((_, reject) => { + if (params.signal!.aborted) { + reject(params.signal!.reason); + return; + } + onAbort = () => reject(params.signal!.reason); + params.signal!.addEventListener("abort", onAbort, { once: true }); + }); + try { + return (await Promise.race([waitPromise, abortPromise]))?.decision; + } finally { + if (onAbort) { + params.signal.removeEventListener("abort", onAbort); + } + } +} + +export function mapExecDecisionToOutcome( + decision: ExecApprovalDecision | null | undefined, +): AppServerApprovalOutcome { + if (decision === "allow-once") { + return "approved-once"; + } + if (decision === "allow-always") { + return "approved-session"; + } + if (decision === null || decision === undefined) { + return "unavailable"; + } + return "denied"; +} + +function truncateForGateway(value: string, maxLength: number): string { + return value.length <= maxLength ? value : `${value.slice(0, Math.max(0, maxLength - 3))}...`; +} diff --git a/extensions/codex/src/app-server/run-attempt.test.ts b/extensions/codex/src/app-server/run-attempt.test.ts index 3f573e1eb04..7c36ee48e53 100644 --- a/extensions/codex/src/app-server/run-attempt.test.ts +++ b/extensions/codex/src/app-server/run-attempt.test.ts @@ -14,6 +14,7 @@ import { } from "../../../../src/plugins/hook-runner-global.js"; import { createMockPluginRegistry } from "../../../../src/plugins/hooks.test-helpers.js"; import { CODEX_GPT5_BEHAVIOR_CONTRACT } from "../../prompt-overlay.js"; +import * as elicitationBridge from "./elicitation-bridge.js"; import type { CodexServerNotification } from "./protocol.js"; import { runCodexAppServerAttempt, __testing } from "./run-attempt.js"; import { writeCodexAppServerBinding } from "./session-binding.js"; @@ -104,6 +105,9 @@ function createAppServerHarness( interval: 1, }); }, + async notify(notification: CodexServerNotification) { + await notify(notification); + }, async completeTurn(params: { threadId: string; turnId: string }) { await notify({ method: "turn/completed", @@ -114,9 +118,6 @@ function createAppServerHarness( }, }); }, - async notify(notification: CodexServerNotification) { - await notify(notification); - }, }; } @@ -621,6 +622,50 @@ describe("runCodexAppServerAttempt", () => { }); }); + it("does not complete on unscoped turn/completed notifications", async () => { + const harness = createStartedThreadHarness(); + const run = runCodexAppServerAttempt( + createParams(path.join(tempDir, "session.jsonl"), path.join(tempDir, "workspace")), + ); + let resolved = false; + void run.then(() => { + resolved = true; + }); + + await harness.waitForMethod("turn/start"); + await harness.notify({ + method: "turn/completed", + params: { + turn: { + id: "turn-1", + status: "completed", + items: [{ type: "agentMessage", id: "msg-wrong", text: "wrong completion" }], + }, + }, + }); + await new Promise((resolve) => setTimeout(resolve, 25)); + expect(resolved).toBe(false); + + await harness.notify({ + method: "turn/completed", + params: { + threadId: "thread-1", + turnId: "turn-1", + turn: { + id: "turn-1", + status: "completed", + items: [{ type: "agentMessage", id: "msg-right", text: "final completion" }], + }, + }, + }); + + await expect(run).resolves.toMatchObject({ + assistantTexts: ["final completion"], + aborted: false, + timedOut: false, + }); + }); + it("releases completion when a projector callback throws during turn/completed", async () => { // Regression for openclaw/openclaw#67996: a throw inside the projector's // turn/completed handler must not strand resolveCompletion, otherwise the @@ -676,6 +721,87 @@ describe("runCodexAppServerAttempt", () => { }); }); + it("routes MCP approval elicitations through the native bridge", async () => { + let notify: (notification: CodexServerNotification) => Promise = async () => undefined; + let handleRequest: + | ((request: { id: string; method: string; params?: unknown }) => Promise) + | undefined; + const bridgeSpy = vi + .spyOn(elicitationBridge, "handleCodexAppServerElicitationRequest") + .mockResolvedValue({ + action: "accept", + content: { approve: true }, + _meta: null, + }); + const request = vi.fn(async (method: string) => { + if (method === "thread/start") { + return { thread: { id: "thread-1" }, model: "gpt-5.4-codex", modelProvider: "openai" }; + } + if (method === "turn/start") { + return { turn: { id: "turn-1", status: "inProgress" } }; + } + return {}; + }); + __testing.setCodexAppServerClientFactoryForTests( + async () => + ({ + request, + addNotificationHandler: (handler: typeof notify) => { + notify = handler; + return () => undefined; + }, + addRequestHandler: ( + handler: (request: { + id: string; + method: string; + params?: unknown; + }) => Promise, + ) => { + handleRequest = handler; + return () => undefined; + }, + }) as never, + ); + + const run = runCodexAppServerAttempt( + createParams(path.join(tempDir, "session.jsonl"), path.join(tempDir, "workspace")), + ); + await vi.waitFor(() => expect(handleRequest).toBeTypeOf("function")); + + const result = await handleRequest?.({ + id: "request-elicitation-1", + method: "mcpServer/elicitation/request", + params: { + threadId: "thread-1", + turnId: "turn-1", + serverName: "codex_apps__github", + mode: "form", + }, + }); + + expect(result).toEqual({ + action: "accept", + content: { approve: true }, + _meta: null, + }); + expect(bridgeSpy).toHaveBeenCalledWith( + expect.objectContaining({ + threadId: "thread-1", + turnId: "turn-1", + }), + ); + + await notify({ + method: "turn/completed", + params: { + threadId: "thread-1", + turnId: "turn-1", + turn: { id: "turn-1", status: "completed" }, + }, + }); + await run; + }); + it("times out app-server startup before thread setup can hang forever", async () => { __testing.setCodexAppServerClientFactoryForTests(() => new Promise(() => undefined)); const params = createParams( diff --git a/extensions/codex/src/app-server/run-attempt.ts b/extensions/codex/src/app-server/run-attempt.ts index 83dc3b2cf7a..ee2245cb839 100644 --- a/extensions/codex/src/app-server/run-attempt.ts +++ b/extensions/codex/src/app-server/run-attempt.ts @@ -31,6 +31,7 @@ import { import { isCodexAppServerApprovalRequest, type CodexAppServerClient } from "./client.js"; import { resolveCodexAppServerRuntimeOptions } from "./config.js"; import { createCodexDynamicToolBridge } from "./dynamic-tools.js"; +import { handleCodexAppServerElicitationRequest } from "./elicitation-bridge.js"; import { CodexAppServerEventProjector } from "./event-projector.js"; import { isJsonObject, @@ -175,7 +176,8 @@ export async function runCodexAppServerAttempt( // inside projector.handleNotification still releases the session lane. // See openclaw/openclaw#67996. const isTurnCompletion = - notification.method === "turn/completed" && isTurnNotification(notification.params, turnId); + notification.method === "turn/completed" && + isTurnNotification(notification.params, thread.threadId, turnId); try { await projector.handleNotification(notification); } catch (error) { @@ -203,6 +205,15 @@ export async function runCodexAppServerAttempt( if (!turnId) { return undefined; } + if (request.method === "mcpServer/elicitation/request") { + return handleCodexAppServerElicitationRequest({ + requestParams: request.params, + paramsForRun: params, + threadId: thread.threadId, + turnId, + signal: runAbortController.signal, + }); + } if (request.method !== "item/tool/call") { if (isCodexAppServerApprovalRequest(request.method)) { return handleApprovalRequest({ @@ -562,16 +573,15 @@ function readDynamicToolCallParams( }; } -function isTurnNotification(value: JsonValue | undefined, turnId: string): boolean { +function isTurnNotification( + value: JsonValue | undefined, + threadId: string, + turnId: string, +): boolean { if (!isJsonObject(value)) { return false; } - const directTurnId = readString(value, "turnId"); - if (directTurnId === turnId) { - return true; - } - const turn = isJsonObject(value.turn) ? value.turn : undefined; - return readString(turn ?? {}, "id") === turnId; + return readString(value, "threadId") === threadId && readString(value, "turnId") === turnId; } function readString(record: JsonObject, key: string): string | undefined { diff --git a/src/auto-reply/get-reply-options.types.ts b/src/auto-reply/get-reply-options.types.ts index 69e07a3d1da..7723da2ac9a 100644 --- a/src/auto-reply/get-reply-options.types.ts +++ b/src/auto-reply/get-reply-options.types.ts @@ -108,6 +108,7 @@ export type GetReplyOptions = { command?: string; host?: string; reason?: string; + scope?: "turn" | "session"; message?: string; }) => Promise | void; /** Called when command output streams or completes. */ diff --git a/src/auto-reply/reply/agent-runner-execution.test.ts b/src/auto-reply/reply/agent-runner-execution.test.ts index 66d9f73e40d..84b8f009fdd 100644 --- a/src/auto-reply/reply/agent-runner-execution.test.ts +++ b/src/auto-reply/reply/agent-runner-execution.test.ts @@ -762,6 +762,7 @@ describe("runAgentTurnWithFallback", () => { command: undefined, host: undefined, reason: undefined, + scope: undefined, message: undefined, }); expect(onCommandOutput).toHaveBeenCalledWith({ diff --git a/src/auto-reply/reply/agent-runner-execution.ts b/src/auto-reply/reply/agent-runner-execution.ts index 8a391b976e4..e59405e02dd 100644 --- a/src/auto-reply/reply/agent-runner-execution.ts +++ b/src/auto-reply/reply/agent-runner-execution.ts @@ -86,6 +86,10 @@ const GPT_CHAT_BREVITY_ACK_MAX_SENTENCES = 3; const GPT_CHAT_BREVITY_SOFT_MAX_CHARS = 900; const GPT_CHAT_BREVITY_SOFT_MAX_SENTENCES = 6; +function readApprovalScopeValue(value: unknown): "turn" | "session" | undefined { + return value === "turn" || value === "session" ? value : undefined; +} + export type RuntimeFallbackAttempt = { provider: string; model: string; @@ -1124,6 +1128,7 @@ export async function runAgentTurnWithFallback(params: { command: readStringValue(evt.data.command), host: readStringValue(evt.data.host), reason: readStringValue(evt.data.reason), + scope: readApprovalScopeValue(evt.data.scope), message: readStringValue(evt.data.message), }); } diff --git a/src/infra/agent-events.ts b/src/infra/agent-events.ts index 70ae8d0c456..339d3673dfa 100644 --- a/src/infra/agent-events.ts +++ b/src/infra/agent-events.ts @@ -68,6 +68,7 @@ export type AgentApprovalEventData = { command?: string; host?: string; reason?: string; + scope?: "turn" | "session"; message?: string; };