Refactor channel approval capability seams (#58634)

Merged via squash.

Prepared head SHA: c9ad4e4706
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Reviewed-by: @gumadeiras

src/auto-reply/reply/commands-approve.ts

@@ -118,6 +118,49 @@ function isApprovalNotFoundError(err: unknown): boolean {
   return /unknown or expired approval id/i.test(err.message);
 }
 
+function formatApprovalSubmitError(error: unknown): string {
+  return error instanceof Error ? error.message : String(error);
+}
+
+type ApprovalMethod = "exec.approval.resolve" | "plugin.approval.resolve";
+
+function resolveApprovalMethods(params: {
+  approvalId: string;
+  execAuthorization: ReturnType<typeof resolveApprovalCommandAuthorization>;
+  pluginAuthorization: ReturnType<typeof resolveApprovalCommandAuthorization>;
+}): ApprovalMethod[] {
+  if (params.approvalId.startsWith("plugin:")) {
+    return params.pluginAuthorization.authorized ? ["plugin.approval.resolve"] : [];
+  }
+  if (params.execAuthorization.authorized && params.pluginAuthorization.authorized) {
+    return ["exec.approval.resolve", "plugin.approval.resolve"];
+  }
+  if (params.execAuthorization.authorized) {
+    return ["exec.approval.resolve"];
+  }
+  if (params.pluginAuthorization.authorized) {
+    return ["plugin.approval.resolve"];
+  }
+  return [];
+}
+
+function resolveApprovalAuthorizationError(params: {
+  approvalId: string;
+  execAuthorization: ReturnType<typeof resolveApprovalCommandAuthorization>;
+  pluginAuthorization: ReturnType<typeof resolveApprovalCommandAuthorization>;
+}): string {
+  if (params.approvalId.startsWith("plugin:")) {
+    return (
+      params.pluginAuthorization.reason ?? "❌ You are not authorized to approve this request."
+    );
+  }
+  return (
+    params.execAuthorization.reason ??
+    params.pluginAuthorization.reason ??
+    "❌ You are not authorized to approve this request."
+  );
+}
+
 export const handleApproveCommand: CommandHandler = async (params, allowTextCommands) => {
   if (!allowTextCommands) {
     return null;
@@ -169,17 +212,6 @@ export const handleApproveCommand: CommandHandler = async (params, allowTextComm
     };
   }
 
-  if (isPluginId && !pluginApprovalAuthorization.authorized) {
-    return {
-      shouldContinue: false,
-      reply: {
-        text:
-          pluginApprovalAuthorization.reason ??
-          "❌ You are not authorized to approve this request.",
-      },
-    };
-  }
-
   const missingScope = requireGatewayClientScopeForInternalChannel(params, {
     label: "/approve",
     allowedScopes: ["operator.approvals", "operator.admin"],
@@ -200,68 +232,49 @@ export const handleApproveCommand: CommandHandler = async (params, allowTextComm
     });
   };
 
-  // Plugin approval IDs are kind-prefixed (`plugin:<uuid>`); route directly when detected.
-  // Unprefixed IDs try the authorized path first, then fall back for backward compat.
-  if (isPluginId) {
-    try {
-      await callApprovalMethod("plugin.approval.resolve");
-    } catch (err) {
-      return {
-        shouldContinue: false,
-        reply: { text: `❌ Failed to submit approval: ${String(err)}` },
-      };
-    }
-  } else if (execApprovalAuthorization.authorized) {
-    try {
-      await callApprovalMethod("exec.approval.resolve");
-    } catch (err) {
-      if (isApprovalNotFoundError(err)) {
-        if (!pluginApprovalAuthorization.authorized) {
-          return {
-            shouldContinue: false,
-            reply: { text: `❌ Failed to submit approval: ${String(err)}` },
-          };
-        }
-        try {
-          await callApprovalMethod("plugin.approval.resolve");
-        } catch (pluginErr) {
-          return {
-            shouldContinue: false,
-            reply: { text: `❌ Failed to submit approval: ${String(pluginErr)}` },
-          };
-        }
-      } else {
-        return {
-          shouldContinue: false,
-          reply: { text: `❌ Failed to submit approval: ${String(err)}` },
-        };
-      }
-    }
-  } else if (pluginApprovalAuthorization.authorized) {
-    try {
-      await callApprovalMethod("plugin.approval.resolve");
-    } catch (err) {
-      if (isApprovalNotFoundError(err)) {
-        return {
-          shouldContinue: false,
-          reply: { text: `❌ Failed to submit approval: ${String(err)}` },
-        };
-      }
-      return {
-        shouldContinue: false,
-        reply: { text: `❌ Failed to submit approval: ${String(err)}` },
-      };
-    }
-  } else {
+  const methods = resolveApprovalMethods({
+    approvalId: parsed.id,
+    execAuthorization: execApprovalAuthorization,
+    pluginAuthorization: pluginApprovalAuthorization,
+  });
+  if (methods.length === 0) {
     return {
       shouldContinue: false,
       reply: {
-        text:
-          execApprovalAuthorization.reason ?? "❌ You are not authorized to approve this request.",
+        text: resolveApprovalAuthorizationError({
+          approvalId: parsed.id,
+          execAuthorization: execApprovalAuthorization,
+          pluginAuthorization: pluginApprovalAuthorization,
+        }),
       },
     };
   }
 
+  let lastError: unknown = null;
+  for (const [index, method] of methods.entries()) {
+    try {
+      await callApprovalMethod(method);
+      lastError = null;
+      break;
+    } catch (error) {
+      lastError = error;
+      const isLastMethod = index === methods.length - 1;
+      if (!isApprovalNotFoundError(error) || isLastMethod) {
+        return {
+          shouldContinue: false,
+          reply: { text: `❌ Failed to submit approval: ${formatApprovalSubmitError(error)}` },
+        };
+      }
+    }
+  }
+
+  if (lastError) {
+    return {
+      shouldContinue: false,
+      reply: { text: `❌ Failed to submit approval: ${formatApprovalSubmitError(lastError)}` },
+    };
+  }
+
   return {
     shouldContinue: false,
     reply: { text: `✅ Approval ${parsed.decision} submitted for ${parsed.id}.` },

src/auto-reply/reply/commands.test.ts

@@ -11,7 +11,10 @@ import {
   buildLegacyDmAccountAllowlistAdapter,
 } from "../../plugin-sdk/allowlist-config-edit.js";
 import { resolveApprovalApprovers } from "../../plugin-sdk/approval-approvers.js";
-import { createApproverRestrictedNativeApprovalAdapter } from "../../plugin-sdk/approval-runtime.js";
+import {
+  createApproverRestrictedNativeApprovalAdapter,
+  createResolvedApproverActionAuthAdapter,
+} from "../../plugin-sdk/approval-runtime.js";
 import { createScopedChannelConfigAdapter } from "../../plugin-sdk/channel-config-helpers.js";
 import { setActivePluginRegistry } from "../../plugins/runtime.js";
 import { DEFAULT_ACCOUNT_ID, normalizeAccountId } from "../../routing/session-key.js";
@@ -111,6 +114,42 @@ const slackCommandTestPlugin: ChannelPlugin = {
   }),
 };
 
+const signalCommandTestPlugin: ChannelPlugin = {
+  ...createChannelTestPluginBase({
+    id: "signal",
+    label: "Signal",
+    docsPath: "/channels/signal",
+    capabilities: {
+      chatTypes: ["direct", "group"],
+      reactions: true,
+      media: true,
+      nativeCommands: true,
+    },
+  }),
+  auth: createResolvedApproverActionAuthAdapter({
+    channelLabel: "Signal",
+    resolveApprovers: ({ cfg, accountId }) => {
+      const signal = accountId ? cfg.channels?.signal?.accounts?.[accountId] : cfg.channels?.signal;
+      return resolveApprovalApprovers({
+        allowFrom: signal?.allowFrom,
+        defaultTo: signal?.defaultTo,
+        normalizeApprover: (value) => String(value).trim() || undefined,
+      });
+    },
+  }),
+  allowlist: buildLegacyDmAccountAllowlistAdapter({
+    channelId: "signal",
+    resolveAccount: ({ cfg, accountId }) =>
+      accountId
+        ? (cfg.channels?.signal?.accounts?.[accountId] ?? {})
+        : (cfg.channels?.signal ?? {}),
+    normalize: ({ values }) => values.map((value) => String(value).trim()).filter(Boolean),
+    resolveDmAllowFrom: (account) => account.allowFrom,
+    resolveGroupPolicy: (account) => account.groupPolicy,
+    resolveGroupOverrides: () => undefined,
+  }),
+};
+
 const whatsappCommandTestPlugin: ChannelPlugin = {
   ...createChannelTestPluginBase({
     id: "whatsapp",
@@ -507,6 +546,11 @@ function setMinimalChannelPluginRegistryForTests(): void {
         plugin: slackCommandTestPlugin,
         source: "test",
       },
+      {
+        pluginId: "signal",
+        plugin: signalCommandTestPlugin,
+        source: "test",
+      },
       {
         pluginId: "telegram",
         plugin: telegramCommandTestPlugin,
@@ -867,6 +911,35 @@ describe("/approve command", () => {
     );
   });
 
+  it("accepts Signal /approve from configured approvers even when chat access is otherwise blocked", async () => {
+    const cfg = {
+      commands: { text: true },
+      channels: {
+        signal: {
+          allowFrom: ["+15551230000"],
+        },
+      },
+    } as OpenClawConfig;
+    const params = buildParams("/approve abc12345 allow-once", cfg, {
+      Provider: "signal",
+      Surface: "signal",
+      SenderId: "+15551230000",
+    });
+    params.command.isAuthorizedSender = false;
+
+    callGatewayMock.mockResolvedValue({ ok: true });
+
+    const result = await handleCommands(params);
+    expect(result.shouldContinue).toBe(false);
+    expect(result.reply?.text).toContain("Approval allow-once submitted");
+    expect(callGatewayMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        method: "exec.approval.resolve",
+        params: { id: "abc12345", decision: "allow-once" },
+      }),
+    );
+  });
+
   it("does not treat implicit default approval auth as a bypass for unauthorized senders", async () => {
     const cfg = {
       commands: { text: true },
@@ -920,7 +993,7 @@ describe("/approve command", () => {
     expect(callGatewayMock).not.toHaveBeenCalled();
   });
 
-  it("accepts Telegram /approve from exec target recipients even when native approvals are disabled", async () => {
+  it("ignores Telegram /approve from exec target recipients when native approvals are disabled", async () => {
     const cfg = {
       commands: { text: true },
       approvals: {
@@ -947,13 +1020,8 @@ describe("/approve command", () => {
 
     const result = await handleCommands(params);
     expect(result.shouldContinue).toBe(false);
-    expect(result.reply?.text).toContain("Approval allow-once submitted");
-    expect(callGatewayMock).toHaveBeenCalledWith(
-      expect.objectContaining({
-        method: "exec.approval.resolve",
-        params: { id: "abc12345", decision: "allow-once" },
-      }),
-    );
+    expect(result.reply).toBeUndefined();
+    expect(callGatewayMock).not.toHaveBeenCalled();
   });
 
   it("requires configured Discord approvers for exec approvals", async () => {
@@ -1188,7 +1256,7 @@ describe("/approve command", () => {
         expectGatewayCalls: 2,
       },
       {
-        name: "telegram approvals disabled",
+        name: "telegram disabled native delivery reports the channel-disabled message",
         cfg: createTelegramApproveCfg(null),
         commandBody: "/approve abc12345 allow-once",
         ctx: {