From 4ea80632036c7126c55498eb33301860249fdc20 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 21 Apr 2026 00:55:50 +0100 Subject: [PATCH] refactor: reuse operator approval gateway lifecycle --- src/gateway/operator-approvals-client.test.ts | 15 ++++ src/infra/approval-gateway-resolver.test.ts | 68 ++------------ src/infra/approval-gateway-resolver.ts | 88 ++++++------------- 3 files changed, 52 insertions(+), 119 deletions(-) diff --git a/src/gateway/operator-approvals-client.test.ts b/src/gateway/operator-approvals-client.test.ts index 4e1a52a6b35..dc2b23fdfe5 100644 --- a/src/gateway/operator-approvals-client.test.ts +++ b/src/gateway/operator-approvals-client.test.ts @@ -106,4 +106,19 @@ describe("withOperatorApprovalsGatewayClient", () => { ), ).rejects.toThrow("gateway closed (1008): pairing required"); }); + + it("falls back to stop when stopAndWait rejects", async () => { + clientState.stopAndWaitSpy.mockRejectedValueOnce(new Error("close failed")); + + await withOperatorApprovalsGatewayClient( + { + config: {} as never, + clientDisplayName: "Matrix approval (@owner:example.org)", + }, + async () => undefined, + ); + + expect(clientState.stopAndWaitSpy).toHaveBeenCalledTimes(1); + expect(clientState.stopSpy).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/infra/approval-gateway-resolver.test.ts b/src/infra/approval-gateway-resolver.test.ts index 49f199d9caf..90633ed0d31 100644 --- a/src/infra/approval-gateway-resolver.test.ts +++ b/src/infra/approval-gateway-resolver.test.ts @@ -2,51 +2,19 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { resolveApprovalOverGateway } from "./approval-gateway-resolver.js"; const hoisted = vi.hoisted(() => ({ - createOperatorApprovalsGatewayClient: vi.fn(), - clientStart: vi.fn(), - clientStop: vi.fn(), - clientStopAndWait: vi.fn(), + withOperatorApprovalsGatewayClient: vi.fn(), clientRequest: vi.fn(), })); vi.mock("../gateway/operator-approvals-client.js", () => ({ - createOperatorApprovalsGatewayClient: hoisted.createOperatorApprovalsGatewayClient, + withOperatorApprovalsGatewayClient: hoisted.withOperatorApprovalsGatewayClient, })); -function createGatewayClient(params: { - stopAndWaitRejects?: boolean; - requestImpl?: typeof hoisted.clientRequest; -}) { - const request = params.requestImpl ?? hoisted.clientRequest; - return { - start: () => { - hoisted.clientStart(); - }, - stop: hoisted.clientStop, - stopAndWait: params.stopAndWaitRejects - ? vi.fn(async () => { - hoisted.clientStopAndWait(); - throw new Error("close failed"); - }) - : vi.fn(async () => { - hoisted.clientStopAndWait(); - }), - request, - }; -} - describe("resolveApprovalOverGateway", () => { beforeEach(() => { - hoisted.clientStart.mockReset(); - hoisted.clientStop.mockReset(); - hoisted.clientStopAndWait.mockReset(); hoisted.clientRequest.mockReset().mockResolvedValue({ ok: true }); - hoisted.createOperatorApprovalsGatewayClient.mockReset().mockImplementation(async (params) => { - const client = createGatewayClient({}); - queueMicrotask(() => { - params.onHelloOk?.({} as never); - }); - return client; + hoisted.withOperatorApprovalsGatewayClient.mockReset().mockImplementation(async (_, run) => { + await run({ request: hoisted.clientRequest }); }); }); @@ -59,19 +27,18 @@ describe("resolveApprovalOverGateway", () => { clientDisplayName: "Discord approval (default)", }); - expect(hoisted.createOperatorApprovalsGatewayClient).toHaveBeenCalledWith( - expect.objectContaining({ + expect(hoisted.withOperatorApprovalsGatewayClient).toHaveBeenCalledWith( + { config: { gateway: { auth: { token: "cfg-token" } } }, gatewayUrl: "ws://gateway.example.test", clientDisplayName: "Discord approval (default)", - }), + }, + expect.any(Function), ); - expect(hoisted.clientStart).toHaveBeenCalledTimes(1); expect(hoisted.clientRequest).toHaveBeenCalledWith("exec.approval.resolve", { id: "approval-1", decision: "allow-once", }); - expect(hoisted.clientStopAndWait).toHaveBeenCalledTimes(1); }); it("routes plugin approvals through plugin.approval.resolve", async () => { @@ -121,23 +88,4 @@ describe("resolveApprovalOverGateway", () => { expect(hoisted.clientRequest).toHaveBeenCalledTimes(1); }); - - it("falls back to stop when stopAndWait rejects", async () => { - hoisted.createOperatorApprovalsGatewayClient.mockReset().mockImplementation(async (params) => { - const client = createGatewayClient({ stopAndWaitRejects: true }); - queueMicrotask(() => { - params.onHelloOk?.({} as never); - }); - return client; - }); - - await resolveApprovalOverGateway({ - cfg: {} as never, - approvalId: "approval-1", - decision: "allow-once", - }); - - expect(hoisted.clientStopAndWait).toHaveBeenCalledTimes(1); - expect(hoisted.clientStop).toHaveBeenCalledTimes(1); - }); }); diff --git a/src/infra/approval-gateway-resolver.ts b/src/infra/approval-gateway-resolver.ts index 9755a79b019..8f1266908d9 100644 --- a/src/infra/approval-gateway-resolver.ts +++ b/src/infra/approval-gateway-resolver.ts @@ -1,5 +1,5 @@ import type { OpenClawConfig } from "../config/types.openclaw.js"; -import { createOperatorApprovalsGatewayClient } from "../gateway/operator-approvals-client.js"; +import { withOperatorApprovalsGatewayClient } from "../gateway/operator-approvals-client.js"; import { isApprovalNotFoundError } from "./approval-errors.js"; import type { ExecApprovalDecision } from "./exec-approvals.js"; @@ -16,64 +16,34 @@ export type ResolveApprovalOverGatewayParams = { export async function resolveApprovalOverGateway( params: ResolveApprovalOverGatewayParams, ): Promise { - let readySettled = false; - let resolveReady!: () => void; - let rejectReady!: (err: unknown) => void; - const ready = new Promise((resolve, reject) => { - resolveReady = resolve; - rejectReady = reject; - }); - const markReady = () => { - if (readySettled) { - return; - } - readySettled = true; - resolveReady(); - }; - const failReady = (err: unknown) => { - if (readySettled) { - return; - } - readySettled = true; - rejectReady(err); - }; - - const gatewayClient = await createOperatorApprovalsGatewayClient({ - config: params.cfg, - gatewayUrl: params.gatewayUrl, - clientDisplayName: - params.clientDisplayName ?? `Approval (${params.senderId?.trim() || "unknown"})`, - onHelloOk: markReady, - onConnectError: failReady, - onClose: (code, reason) => { - failReady(new Error(`gateway closed (${code}): ${reason}`)); + await withOperatorApprovalsGatewayClient( + { + config: params.cfg, + gatewayUrl: params.gatewayUrl, + clientDisplayName: + params.clientDisplayName ?? `Approval (${params.senderId?.trim() || "unknown"})`, }, - }); - - try { - gatewayClient.start(); - await ready; - const requestResolve = async (method: "exec.approval.resolve" | "plugin.approval.resolve") => { - await gatewayClient.request(method, { - id: params.approvalId, - decision: params.decision, - }); - }; - if (params.approvalId.startsWith("plugin:")) { - await requestResolve("plugin.approval.resolve"); - return; - } - try { - await requestResolve("exec.approval.resolve"); - } catch (err) { - if (!params.allowPluginFallback || !isApprovalNotFoundError(err)) { - throw err; + async (gatewayClient) => { + const requestResolve = async ( + method: "exec.approval.resolve" | "plugin.approval.resolve", + ) => { + await gatewayClient.request(method, { + id: params.approvalId, + decision: params.decision, + }); + }; + if (params.approvalId.startsWith("plugin:")) { + await requestResolve("plugin.approval.resolve"); + return; } - await requestResolve("plugin.approval.resolve"); - } - } finally { - await gatewayClient.stopAndWait().catch(() => { - gatewayClient.stop(); - }); - } + try { + await requestResolve("exec.approval.resolve"); + } catch (err) { + if (!params.allowPluginFallback || !isApprovalNotFoundError(err)) { + throw err; + } + await requestResolve("plugin.approval.resolve"); + } + }, + ); }