diff --git a/packages/gateway-protocol/src/schema/approvals.ts b/packages/gateway-protocol/src/schema/approvals.ts index bc3dddf90773..c62746797bd5 100644 --- a/packages/gateway-protocol/src/schema/approvals.ts +++ b/packages/gateway-protocol/src/schema/approvals.ts @@ -251,6 +251,7 @@ export const ApprovalChannelReviewerSchema = closedObject({ export const ApprovalResolveParamsSchema = closedObject({ id: ApprovalRecordCommonFields.id, + instanceId: Type.Optional(NonEmptyString), kind: ApprovalKindSchema, decision: ApprovalDecisionSchema, reviewer: Type.Optional(ApprovalChannelReviewerSchema), diff --git a/packages/gateway-protocol/src/schema/exec-approvals.ts b/packages/gateway-protocol/src/schema/exec-approvals.ts index 7d7df43c9ee4..82dd44343a39 100644 --- a/packages/gateway-protocol/src/schema/exec-approvals.ts +++ b/packages/gateway-protocol/src/schema/exec-approvals.ts @@ -313,6 +313,7 @@ export const ExecApprovalRequestParamsSchema = closedObject({ /** Reviewer decision payload for one pending exec approval. */ export const ExecApprovalResolveParamsSchema = closedObject({ id: NonEmptyString, + instanceId: Type.Optional(NonEmptyString), decision: NonEmptyString, reviewer: Type.Optional(ApprovalChannelReviewerSchema), }); diff --git a/packages/gateway-protocol/src/schema/plugin-approvals.ts b/packages/gateway-protocol/src/schema/plugin-approvals.ts index a4226c284da1..6f9a79847cea 100644 --- a/packages/gateway-protocol/src/schema/plugin-approvals.ts +++ b/packages/gateway-protocol/src/schema/plugin-approvals.ts @@ -56,6 +56,7 @@ export const PluginApprovalRequestParamsSchema = closedObject({ /** Reviewer decision payload resolving one pending plugin approval request. */ export const PluginApprovalResolveParamsSchema = closedObject({ id: NonEmptyString, + instanceId: Type.Optional(NonEmptyString), decision: NonEmptyString, reviewer: Type.Optional(ApprovalChannelReviewerSchema), }); diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index c43e75a941ad..882eeb9e046c 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -82,6 +82,7 @@ type ExecApprovalResolutionSource = "operator" | "auto-review"; export type ExecApprovalRecord = { id: string; + instanceId: string; request: TPayload; createdAtMs: number; expiresAtMs: number; @@ -267,6 +268,7 @@ export class ExecApprovalManager { const resolvedId = hasExplicitId ? id : randomUUID(); const record: ExecApprovalRecord = { id: resolvedId, + instanceId: randomUUID(), request, createdAtMs: now, expiresAtMs, diff --git a/src/gateway/node-invoke-system-run-approval.test.ts b/src/gateway/node-invoke-system-run-approval.test.ts index e14074f30f3b..330857ffcaac 100644 --- a/src/gateway/node-invoke-system-run-approval.test.ts +++ b/src/gateway/node-invoke-system-run-approval.test.ts @@ -125,6 +125,7 @@ describe("sanitizeSystemRunParamsForForwarding", () => { const effectiveBindingArgv = bindingArgv ?? commandArgv ?? [command]; return { id: "approval-1", + instanceId: "approval-instance-1", request: { host: "node", nodeId: "node-1", diff --git a/src/gateway/server-methods/approval-record-lookup.ts b/src/gateway/server-methods/approval-record-lookup.ts index 13fff7baca27..850db4d77d89 100644 --- a/src/gateway/server-methods/approval-record-lookup.ts +++ b/src/gateway/server-methods/approval-record-lookup.ts @@ -127,6 +127,7 @@ export function listVisiblePendingApprovalRequests(params: { }): Array<{ approvalKind?: ChannelApprovalKind; id: string; + instanceId: string; request: TPayload; createdAtMs: number; expiresAtMs: number; @@ -140,8 +141,8 @@ export function listVisiblePendingApprovalRequests(params: { ...(params.cfg ? { cfg: params.cfg } : {}), }), ) - .map(({ id, request, createdAtMs, expiresAtMs }) => { - const approval = { id, request, createdAtMs, expiresAtMs }; + .map(({ id, instanceId, request, createdAtMs, expiresAtMs }) => { + const approval = { id, instanceId, request, createdAtMs, expiresAtMs }; return params.approvalKind ? Object.assign(approval, { approvalKind: params.approvalKind }) : approval; diff --git a/src/gateway/server-methods/approval-shared.test.ts b/src/gateway/server-methods/approval-shared.test.ts index 5f1ad0832e91..a1d704529436 100644 --- a/src/gateway/server-methods/approval-shared.test.ts +++ b/src/gateway/server-methods/approval-shared.test.ts @@ -1286,6 +1286,34 @@ describe("handlePendingApprovalRequest", () => { expect(manager.getSnapshot(record.id)?.decision).toBeUndefined(); }); + it("rejects a decision bound to an older approval instance", async () => { + const manager = new ExecApprovalManager(); + const record = manager.create({ command: "echo replacement" }, 60_000, "approval-reused"); + void manager.register(record, 60_000); + const respond = vi.fn(); + + await handleApprovalResolve({ + approvalKind: "exec", + manager, + inputId: record.id, + instanceId: "older-instance", + decision: "allow-once", + respond, + context: { + broadcast: vi.fn(), + broadcastToConnIds: vi.fn(), + } as unknown as GatewayRequestContext, + client: null, + }); + + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ message: "unknown or expired approval id" }), + ); + expect(manager.getSnapshot(record.id)?.decision).toBeUndefined(); + }); + it("does not wait on decisions for approvals hidden from the caller", async () => { const manager = new ExecApprovalManager(); const record = manager.create( diff --git a/src/gateway/server-methods/approval-shared.ts b/src/gateway/server-methods/approval-shared.ts index fc31cdc244d9..a0e2577544a0 100644 --- a/src/gateway/server-methods/approval-shared.ts +++ b/src/gateway/server-methods/approval-shared.ts @@ -58,6 +58,7 @@ type RequestedApprovalEvent< > = { approvalKind?: TKind; id: string; + instanceId?: string; request: TPayload; createdAtMs: number; expiresAtMs: number; @@ -75,6 +76,7 @@ type ApprovalRequestDeliveryRoute = "approval-client" | "forwarder" | "turn-sour type ApprovalResolveParams = { id: string; + instanceId?: string; decision: string; reviewer?: ApprovalChannelReviewer; }; @@ -153,6 +155,7 @@ export function buildRequestedApprovalEvent< return { ...(approvalKind ? { approvalKind } : {}), id: record.id, + instanceId: record.instanceId, request: record.request, createdAtMs: record.createdAtMs, expiresAtMs: record.expiresAtMs, @@ -167,6 +170,7 @@ export function resolveApprovalDecisionParams; inputId: string; + instanceId?: string; decision: ExecApprovalDecision; respond: RespondFn; context: GatewayRequestContext; @@ -560,6 +566,11 @@ export async function handleApprovalResolve< return; } + if (params.instanceId && resolved.snapshot.instanceId !== params.instanceId) { + respondUnknownOrExpiredApproval(params.respond); + return; + } + const validationError = params.validateDecision?.(resolved.snapshot); if (validationError) { params.respond( diff --git a/src/gateway/server-methods/approval.ts b/src/gateway/server-methods/approval.ts index db6d0aeed39d..87844853a4b6 100644 --- a/src/gateway/server-methods/approval.ts +++ b/src/gateway/server-methods/approval.ts @@ -435,7 +435,7 @@ export function createApprovalHandlers( ? params.execApprovalManager.getLiveSnapshot(record.id) : record.kind === "plugin" ? params.pluginApprovalManager.getLiveSnapshot(record.id) - : undefined; + : params.systemAgentApprovalManager?.getLiveSnapshot(record.id); if (resolveParams?.reviewer && (!custody || !liveRecord || !custody.authorizes(liveRecord))) { respondApprovalNotFound(respond); return; @@ -454,6 +454,10 @@ export function createApprovalHandlers( respond(true, { applied: false, approval }, undefined); return; } + if (resolveParams?.instanceId && liveRecord?.instanceId !== resolveParams.instanceId) { + respondApprovalNotFound(respond); + return; + } const resolver = custody ? ({ kind: "channel", id: custody.resolverId } as const) : resolveApprovalResolver(client); diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index c31a6528a571..9d76ab19dec5 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -457,7 +457,7 @@ export function createExecApprovalHandlers( if (!decisionPromise) { return; } - const requestEvent: ExecApprovalRequest = buildRequestedApprovalEvent(record, "exec"); + const requestEvent = buildRequestedApprovalEvent(record, "exec"); const forwardRequest = opts?.forwarder?.handleRequested.bind(opts.forwarder); const iosPushRequest = opts?.iosPushDelivery?.handleRequested?.bind(opts.iosPushDelivery); await handlePendingApprovalRequest({ @@ -522,12 +522,13 @@ export function createExecApprovalHandlers( if (!resolveParams) { return; } - const { inputId, decision, reviewer } = resolveParams; + const { inputId, instanceId, decision, reviewer } = resolveParams; let autoReviewResolution = false; await handleApprovalResolve({ approvalKind: "exec", manager, inputId, + instanceId, decision, respond, context, diff --git a/src/gateway/server-methods/plugin-approval.ts b/src/gateway/server-methods/plugin-approval.ts index 25a671ff455e..97ac5d4f42d9 100644 --- a/src/gateway/server-methods/plugin-approval.ts +++ b/src/gateway/server-methods/plugin-approval.ts @@ -310,11 +310,12 @@ export function createPluginApprovalHandlers( if (!resolveParams) { return; } - const { inputId, decision, reviewer } = resolveParams; + const { inputId, instanceId, decision, reviewer } = resolveParams; await handleApprovalResolve({ approvalKind: "plugin", manager, inputId, + instanceId, decision, respond, context, diff --git a/src/infra/exec-approvals-core.ts b/src/infra/exec-approvals-core.ts index 65eaacc1ff7b..e1cbd8b09f1d 100644 --- a/src/infra/exec-approvals-core.ts +++ b/src/infra/exec-approvals-core.ts @@ -219,6 +219,7 @@ export type ExecApprovalRequest = { /** Descriptive wire metadata; readers derive it from the payload when absent. */ approvalKind?: "exec"; id: string; + instanceId?: string; request: ExecApprovalRequestPayload; createdAtMs: number; expiresAtMs: number; diff --git a/ui/src/app/exec-approval.ts b/ui/src/app/exec-approval.ts index c2f2e4ee6dde..5889681111ee 100644 --- a/ui/src/app/exec-approval.ts +++ b/ui/src/app/exec-approval.ts @@ -23,6 +23,8 @@ export type ExecApprovalDecision = "allow-once" | "allow-always" | "deny"; export type ExecApprovalRequest = { id: string; + /** Gateway-owned lifecycle identity. Older event producers omit it. */ + instanceId?: string; kind: "exec" | "plugin" | "system-agent"; request: ExecApprovalRequestPayload; pluginTitle?: string; @@ -124,6 +126,7 @@ function parseExecApprovalRequested(payload: unknown): ExecApprovalRequest | nul } return { id, + instanceId: normalizeOptionalString(payload.instanceId) ?? undefined, kind: "exec", request: { command, @@ -192,6 +195,7 @@ function parsePluginApprovalRequested(payload: unknown): ExecApprovalRequest | n return { id, + instanceId: normalizeOptionalString(payload.instanceId) ?? undefined, kind: "plugin", request: { command: title, @@ -225,6 +229,7 @@ function parseSystemAgentApprovalRequested(payload: unknown): ExecApprovalReques } return { id, + instanceId: normalizeOptionalString(payload.instanceId) ?? undefined, kind: "system-agent", request: { command, @@ -263,13 +268,18 @@ export async function resolveApprovalRequest( if (approval.kind === "system-agent") { await client.request("approval.resolve", { id: approval.id, + ...(approval.instanceId ? { instanceId: approval.instanceId } : {}), kind: "system-agent", decision, }); return; } const method = approval.kind === "plugin" ? "plugin.approval.resolve" : "exec.approval.resolve"; - await client.request(method, { id: approval.id, decision }); + await client.request(method, { + id: approval.id, + ...(approval.instanceId ? { instanceId: approval.instanceId } : {}), + decision, + }); } function pruneExecApprovalQueue(queue: ExecApprovalRequest[]): ExecApprovalRequest[] { diff --git a/ui/src/app/overlays-access.test-support.ts b/ui/src/app/overlays-access.test-support.ts index 6f6e24531047..d4faa3d0659c 100644 --- a/ui/src/app/overlays-access.test-support.ts +++ b/ui/src/app/overlays-access.test-support.ts @@ -22,9 +22,10 @@ export function deferred() { return { promise, reject, resolve }; } -export function approval(id: string, createdAtMs: number) { +export function approval(id: string, createdAtMs: number, instanceId = `${id}:${createdAtMs}`) { return { id, + instanceId, createdAtMs, expiresAtMs: Date.now() + 60_000, request: { command: `echo ${id}` }, @@ -79,10 +80,10 @@ export function createGatewayHarness( listener(frame); } }, - emitApproval(id: string, createdAtMs: number) { + emitApproval(id: string, createdAtMs: number, instanceId?: string) { const event: GatewayEventFrame = { event: "exec.approval.requested", - payload: approval(id, createdAtMs), + payload: approval(id, createdAtMs, instanceId), type: "event", }; for (const listener of eventListeners) { @@ -104,6 +105,7 @@ export function createGatewayHarness( event: "openclaw.approval.requested", payload: { id, + instanceId: `${id}:${createdAtMs}`, createdAtMs, expiresAtMs: Date.now() + 60_000, request: { diff --git a/ui/src/app/overlays.approval-replacement.test.ts b/ui/src/app/overlays.approval-replacement.test.ts index e53edc381dfc..7ca12ba94154 100644 --- a/ui/src/app/overlays.approval-replacement.test.ts +++ b/ui/src/app/overlays.approval-replacement.test.ts @@ -8,7 +8,7 @@ import { } from "./overlays-access.test-support.ts"; import { createApplicationOverlays } from "./overlays.ts"; -function approvalRace(id: string, replacementCreatedAtMs: number) { +function approvalRace(id: string, replacementInstanceId: string) { const resolveAttempt = deferred(); const request = vi.fn((method) => method.endsWith(".list") ? Promise.resolve([]) : resolveAttempt.promise, @@ -18,7 +18,7 @@ function approvalRace(id: string, replacementCreatedAtMs: number) { harness.emitApproval(id, 1_000); const original = overlays.snapshot.approvalQueue[0]; const decision = overlays.decideApproval("allow-once"); - harness.emitApproval(id, replacementCreatedAtMs); + harness.emitApproval(id, 1_000, replacementInstanceId); return { decision, original, overlays, request, resolveAttempt }; } @@ -27,7 +27,7 @@ describe("application approval replacement races", () => { it(`keeps a refreshed approval owned by its pending decision after ${outcome}`, async () => { const { decision, original, overlays, resolveAttempt } = approvalRace( "approval-refreshed", - 1_000, + "approval-refreshed:1000", ); expect(overlays.snapshot.approvalQueue[0]).not.toBe(original); @@ -69,7 +69,7 @@ describe("application approval replacement races", () => { async ({ error, stale }) => { const { decision, overlays, request, resolveAttempt } = approvalRace( "approval-reused", - 2_000, + "replacement-instance", ); const listRequestCount = request.mock.calls.filter(([method]) => method.endsWith(".list"), @@ -83,7 +83,7 @@ describe("application approval replacement races", () => { await decision; expect(overlays.snapshot.approvalQueue).toEqual([ - expect.objectContaining({ createdAtMs: 2_000, id: "approval-reused" }), + expect.objectContaining({ instanceId: "replacement-instance", id: "approval-reused" }), ]); expect(overlays.snapshot.approvalErrors).toEqual(new Map()); if (stale) { diff --git a/ui/src/app/overlays.ts b/ui/src/app/overlays.ts index 701d0f101b86..c30a45fc0b5b 100644 --- a/ui/src/app/overlays.ts +++ b/ui/src/app/overlays.ts @@ -616,14 +616,16 @@ export function createApplicationOverlays( operation.grantGeneration === approvalGrantGeneration && readGatewayOperatorAccess(gateway.snapshot).canGrantApprovals && isCurrentClient(operation.client); - // A refresh can reconstruct the same approval, while a reused id - // with a different creation time denotes a new protected request. + // The Gateway owns this lifecycle token. Approval ids are reusable, so + // an older decision must never settle or annotate a replacement record. const isCurrentApproval = () => promptState.execApprovalQueue.some( (entry) => entry.id === active.id && entry.kind === active.kind && - entry.createdAtMs === active.createdAtMs, + (active.instanceId + ? entry.instanceId === active.instanceId + : !entry.instanceId && entry.createdAtMs === active.createdAtMs), ); publish(); try { diff --git a/ui/src/e2e/approval-flow.e2e.test.ts b/ui/src/e2e/approval-flow.e2e.test.ts index a331dbadb4ea..f5f2f72cd803 100644 --- a/ui/src/e2e/approval-flow.e2e.test.ts +++ b/ui/src/e2e/approval-flow.e2e.test.ts @@ -17,9 +17,16 @@ const activeSessionKey = "agent:main:main"; const captureUiProof = process.env.OPENCLAW_CAPTURE_UI_PROOF === "1"; const proofDir = path.join(process.cwd(), ".artifacts", "control-ui-e2e", "approval-flow"); -function approval(id: string, command: string, createdAtMs: number, sessionKey = activeSessionKey) { +function approval( + id: string, + command: string, + createdAtMs: number, + sessionKey = activeSessionKey, + instanceId = `${id}:${createdAtMs}`, +) { return { id, + instanceId, createdAtMs, expiresAtMs: Date.now() + 60_000, request: { command, agentId: "main", sessionKey }, @@ -120,7 +127,13 @@ suite.define(() => { .toBe(0); await gateway.emitGatewayEvent( "exec.approval.requested", - approval("approval-reused", "echo replacement approval", 2_000), + approval( + "approval-reused", + "echo replacement approval", + 1_000, + activeSessionKey, + "replacement-instance", + ), ); const replacement = currentPage.getByText("echo replacement approval", { exact: true }); await replacement.waitFor(); diff --git a/ui/src/test-helpers/app-sidebar-cases/attention.ts b/ui/src/test-helpers/app-sidebar-cases/attention.ts index 5e7d237d9a07..185a5527de88 100644 --- a/ui/src/test-helpers/app-sidebar-cases/attention.ts +++ b/ui/src/test-helpers/app-sidebar-cases/attention.ts @@ -242,6 +242,7 @@ describe("AppSidebar session attention", () => { it("shows approval attention ahead of a run error", async () => { const approval = { id: "approval-1", + instanceId: "approval-instance-1", kind: "exec", request: { command: "git status", sessionKey }, createdAtMs: Date.now(), @@ -266,6 +267,7 @@ describe("AppSidebar session attention", () => { const mainKey = "agent:main:main"; const approval = { id: "approval-main", + instanceId: "approval-main-instance", kind: "exec", request: { command: "git status", sessionKey: mainKey }, createdAtMs: Date.now(), @@ -387,6 +389,7 @@ describe("AppSidebar session attention", () => { ]); const approval = { id: "approval-child", + instanceId: "approval-child-instance", kind: "exec", request: { command: "git status", sessionKey: childKey }, createdAtMs: Date.now(),