From 52fb4a149a90de2fa4d43c77d4cf9a44bdecdc4a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 30 Mar 2026 08:02:37 +0900 Subject: [PATCH] refactor: share approval interactive renderers --- extensions/discord/src/channel.test.ts | 145 ------------- extensions/discord/src/channel.ts | 196 +----------------- extensions/telegram/src/button-types.ts | 18 +- extensions/telegram/src/channel.test.ts | 51 +---- extensions/telegram/src/channel.ts | 32 +-- .../telegram/src/exec-approval-forwarding.ts | 14 +- .../telegram/src/inline-buttons.test.ts | 16 ++ src/infra/exec-approval-forwarder.test.ts | 44 ++-- src/infra/exec-approval-forwarder.ts | 19 +- src/infra/exec-approval-reply.test.ts | 24 +++ src/infra/exec-approval-reply.ts | 53 ++++- src/infra/plugin-approval-forwarder.test.ts | 42 ++-- src/plugin-sdk/approval-renderers.test.ts | 63 +++++- src/plugin-sdk/approval-renderers.ts | 62 ++++-- src/plugin-sdk/approval-runtime.ts | 1 + 15 files changed, 289 insertions(+), 491 deletions(-) diff --git a/extensions/discord/src/channel.test.ts b/extensions/discord/src/channel.test.ts index 3170248f71d..4ee1818e686 100644 --- a/extensions/discord/src/channel.test.ts +++ b/extensions/discord/src/channel.test.ts @@ -1,7 +1,3 @@ -import type { - PluginApprovalRequest, - PluginApprovalResolved, -} from "openclaw/plugin-sdk/infra-runtime"; import type { PluginRuntime } from "openclaw/plugin-sdk/testing"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { createStartAccountContext } from "../../../test/helpers/plugins/start-account-context.js"; @@ -49,38 +45,6 @@ function createCfg(): OpenClawConfig { } as OpenClawConfig; } -function createPluginApprovalRequest( - overrides?: Partial, -): PluginApprovalRequest { - return { - id: "plugin:approval-1", - request: { - title: "Sensitive plugin action", - description: "The plugin asked to perform a sensitive action.", - severity: "warning", - pluginId: "plugin-test", - toolName: "plugin.tool", - agentId: "agent-1", - sessionKey: "agent:agent-1:discord:channel:123456789", - ...overrides, - }, - createdAtMs: 1_000, - expiresAtMs: 61_000, - }; -} - -function createPluginApprovalResolved( - request?: PluginApprovalRequest["request"], -): PluginApprovalResolved { - return { - id: "plugin:approval-1", - decision: "allow-once", - resolvedBy: "discord:123", - ts: 2_000, - request, - }; -} - function resolveAccount(cfg: OpenClawConfig): ResolvedDiscordAccount { return discordPlugin.config.resolveAccount(cfg, "default") as ResolvedDiscordAccount; } @@ -148,115 +112,6 @@ describe("discordPlugin outbound", () => { expect(result).toMatchObject({ channel: "discord", messageId: "m1" }); }); - it("builds interactive plugin approval pending payloads for Discord forwarding", () => { - const cfg = createCfg(); - cfg.channels!.discord!.execApprovals = { - enabled: true, - approvers: ["123"], - }; - const payload = discordPlugin.execApprovals?.render?.plugin?.buildPendingPayload?.({ - cfg, - request: createPluginApprovalRequest(), - target: { channel: "discord", to: "user:123" }, - nowMs: 2_000, - }); - - expect(payload?.text).toContain("Plugin approval required"); - const discordData = (payload?.channelData as { discord?: { components?: unknown } } | undefined) - ?.discord; - expect(discordData?.components).toBeDefined(); - const componentsJson = JSON.stringify(discordData?.components ?? {}); - expect(componentsJson).toContain("Plugin Approval Required"); - expect(componentsJson).toContain("execapproval:id=plugin%3Aapproval-1;action=allow-once"); - const execApproval = (payload?.channelData as { execApproval?: { approvalId?: string } }) - ?.execApproval; - expect(execApproval?.approvalId).toBe("plugin:approval-1"); - }); - - it("neutralizes plugin approval mentions in forwarded text and components", () => { - const cfg = createCfg(); - cfg.channels!.discord!.execApprovals = { - enabled: true, - approvers: ["123"], - }; - const payload = discordPlugin.execApprovals?.render?.plugin?.buildPendingPayload?.({ - cfg, - request: createPluginApprovalRequest({ - title: "Heads up @everyone <@123> <@&456>", - description: "route @here and <#789>", - }), - target: { channel: "discord", to: "user:123" }, - nowMs: 2_000, - }); - - const text = payload?.text ?? ""; - const componentsJson = JSON.stringify( - ((payload?.channelData as { discord?: { components?: unknown } } | undefined)?.discord - ?.components ?? {}) as object, - ); - - expect(text).toContain("@\u200beveryone"); - expect(text).toContain("@\u200bhere"); - expect(text).toContain("<@\u200b123>"); - expect(text).toContain("<@\u200b&456>"); - expect(text).toContain("<#\u200b789>"); - expect(text).not.toContain("@everyone"); - expect(text).not.toContain("@here"); - expect(componentsJson).not.toContain("@everyone"); - expect(componentsJson).not.toContain("@here"); - expect(componentsJson).not.toContain("<@123>"); - expect(componentsJson).not.toContain("<@&456>"); - expect(componentsJson).not.toContain("<#789>"); - }); - - it("falls back to non-interactive plugin approval pending payload when Discord exec approvals are disabled", () => { - const payload = discordPlugin.execApprovals?.render?.plugin?.buildPendingPayload?.({ - cfg: createCfg(), - request: createPluginApprovalRequest(), - target: { channel: "discord", to: "user:123" }, - nowMs: 2_000, - }); - - expect(payload?.text).toContain("Plugin approval required"); - const channelData = payload?.channelData as - | { - execApproval?: { approvalId?: string; approvalSlug?: string }; - discord?: { components?: unknown }; - } - | undefined; - expect(channelData?.execApproval?.approvalId).toBe("plugin:approval-1"); - expect(channelData?.execApproval?.approvalSlug).toBe("plugin:a"); - expect(channelData?.discord?.components).toBeUndefined(); - }); - - it("builds rich plugin approval resolved payloads when request snapshot is available", () => { - const payload = discordPlugin.execApprovals?.render?.plugin?.buildResolvedPayload?.({ - cfg: createCfg(), - resolved: createPluginApprovalResolved(createPluginApprovalRequest().request), - target: { channel: "discord", to: "user:123" }, - }); - - expect(payload?.text).toContain("Plugin approval allowed once"); - const discordData = (payload?.channelData as { discord?: { components?: unknown } } | undefined) - ?.discord; - expect(discordData?.components).toBeDefined(); - const componentsJson = JSON.stringify(discordData?.components ?? {}); - expect(componentsJson).toContain("Plugin Approval: Allowed (once)"); - }); - - it("falls back to plain text plugin resolved payload when request snapshot is missing", () => { - const payload = discordPlugin.execApprovals?.render?.plugin?.buildResolvedPayload?.({ - cfg: createCfg(), - resolved: createPluginApprovalResolved(undefined), - target: { channel: "discord", to: "user:123" }, - }); - - expect(payload?.text).toContain("Plugin approval allowed once"); - const discordData = (payload?.channelData as { discord?: { components?: unknown } } | undefined) - ?.discord; - expect(discordData?.components).toBeUndefined(); - }); - it("uses direct Discord probe helpers for status probes", async () => { const runtimeProbeDiscord = vi.fn(async () => { throw new Error("runtime Discord probe should not be used"); diff --git a/extensions/discord/src/channel.ts b/extensions/discord/src/channel.ts index b50de6213a9..f29658032f6 100644 --- a/extensions/discord/src/channel.ts +++ b/extensions/discord/src/channel.ts @@ -4,15 +4,7 @@ import { createAccountScopedAllowlistNameResolver, createNestedAllowlistOverrideResolver, } from "openclaw/plugin-sdk/allowlist-config-edit"; -import { - buildPluginApprovalPendingReplyPayload, - createApproverRestrictedNativeApprovalAdapter, - buildPluginApprovalRequestMessage, - buildPluginApprovalResolvedMessage, - buildPluginApprovalResolvedReplyPayload, - type PluginApprovalRequest, - type PluginApprovalResolved, -} from "openclaw/plugin-sdk/approval-runtime"; +import { createApproverRestrictedNativeApprovalAdapter } from "openclaw/plugin-sdk/approval-runtime"; import { createScopedDmSecurityResolver } from "openclaw/plugin-sdk/channel-config-helpers"; import { createPairingPrefixStripper } from "openclaw/plugin-sdk/channel-pairing"; import { createOpenProviderConfiguredRouteWarningCollector } from "openclaw/plugin-sdk/channel-policy"; @@ -37,7 +29,6 @@ import { type ResolvedDiscordAccount, } from "./accounts.js"; import { auditDiscordChannelPermissions, collectDiscordAuditChannelIds } from "./audit.js"; -import type { DiscordComponentMessageSpec } from "./components.js"; import { listDiscordDirectoryGroupsFromConfig, listDiscordDirectoryPeersFromConfig, @@ -100,7 +91,6 @@ async function loadDiscordProbeRuntime() { const meta = getChatChannelMeta("discord"); const REQUIRED_DISCORD_PERMISSIONS = ["ViewChannel", "SendMessages"] as const; -const DISCORD_EXEC_APPROVAL_KEY = "execapproval"; const resolveDiscordDmPolicy = createScopedDmSecurityResolver({ channelKey: "discord", @@ -129,147 +119,6 @@ function formatDiscordIntents(intents?: { ].join(" "); } -function encodeCustomIdValue(value: string): string { - return encodeURIComponent(value); -} - -function buildDiscordExecApprovalCustomId( - approvalId: string, - action: "allow-once" | "allow-always" | "deny", -): string { - return [ - `${DISCORD_EXEC_APPROVAL_KEY}:id=${encodeCustomIdValue(approvalId)}`, - `action=${action}`, - ].join(";"); -} - -function formatDiscordApprovalPreview(value: string, maxChars: number): string { - const trimmed = value - .replace(/@everyone/gi, "@\u200beveryone") - .replace(/@here/gi, "@\u200bhere") - .replace(/<@/g, "<@\u200b") - .replace(/<#/g, "<#\u200b") - .trim(); - const raw = trimmed.length > maxChars ? `${trimmed.slice(0, maxChars)}...` : trimmed; - return raw.replace(/`/g, "\u200b`"); -} - -function buildDiscordPluginPendingComponentSpec(params: { - request: PluginApprovalRequest; -}): DiscordComponentMessageSpec { - const request = params.request.request; - const severity = request.severity ?? "warning"; - const severityLabel = - severity === "critical" ? "Critical" : severity === "info" ? "Info" : "Warning"; - const accentColor = - severity === "critical" ? "#ED4245" : severity === "info" ? "#5865F2" : "#FAA61A"; - const expiresAtSeconds = Math.max(0, Math.floor(params.request.expiresAtMs / 1000)); - const metadataLines: string[] = [`- Severity: ${severityLabel}`]; - if (request.toolName) { - metadataLines.push(`- Tool: ${request.toolName}`); - } - if (request.pluginId) { - metadataLines.push(`- Plugin: ${request.pluginId}`); - } - if (request.agentId) { - metadataLines.push(`- Agent: ${request.agentId}`); - } - return { - container: { accentColor }, - blocks: [ - { type: "text", text: "## Plugin Approval Required" }, - { type: "text", text: "A plugin action needs your approval." }, - { type: "separator", divider: true, spacing: "small" }, - { - type: "text", - text: `### Title\n\`\`\`\n${formatDiscordApprovalPreview(request.title, 500)}\n\`\`\``, - }, - { - type: "text", - text: `### Description\n${formatDiscordApprovalPreview(request.description, 1000)}`, - }, - { type: "text", text: metadataLines.join("\n") }, - { - type: "actions", - buttons: [ - { - label: "Allow once", - style: "success", - internalCustomId: buildDiscordExecApprovalCustomId(params.request.id, "allow-once"), - }, - { - label: "Always allow", - style: "primary", - internalCustomId: buildDiscordExecApprovalCustomId(params.request.id, "allow-always"), - }, - { - label: "Deny", - style: "danger", - internalCustomId: buildDiscordExecApprovalCustomId(params.request.id, "deny"), - }, - ], - }, - { type: "separator", divider: false, spacing: "small" }, - { type: "text", text: `-# Expires ยท ID: ${params.request.id}` }, - ], - }; -} - -function buildDiscordPluginResolvedComponentSpec(params: { - resolved: PluginApprovalResolved; -}): DiscordComponentMessageSpec | undefined { - const request = params.resolved.request; - if (!request) { - return undefined; - } - const decisionLabel = - params.resolved.decision === "allow-once" - ? "Allowed (once)" - : params.resolved.decision === "allow-always" - ? "Allowed (always)" - : "Denied"; - const accentColor = - params.resolved.decision === "deny" - ? "#ED4245" - : params.resolved.decision === "allow-always" - ? "#5865F2" - : "#57F287"; - const metadataLines: string[] = []; - if (request.toolName) { - metadataLines.push(`- Tool: ${request.toolName}`); - } - if (request.pluginId) { - metadataLines.push(`- Plugin: ${request.pluginId}`); - } - if (request.agentId) { - metadataLines.push(`- Agent: ${request.agentId}`); - } - return { - container: { accentColor }, - blocks: [ - { type: "text", text: `## Plugin Approval: ${decisionLabel}` }, - { - type: "text", - text: params.resolved.resolvedBy ? `Resolved by ${params.resolved.resolvedBy}` : "Resolved", - }, - { type: "separator", divider: true, spacing: "small" }, - { - type: "text", - text: `### Title\n\`\`\`\n${formatDiscordApprovalPreview(request.title, 500)}\n\`\`\``, - }, - { - type: "text", - text: `### Description\n${formatDiscordApprovalPreview(request.description, 1000)}`, - }, - ...(metadataLines.length > 0 - ? [{ type: "text" as const, text: metadataLines.join("\n") }] - : []), - { type: "separator", divider: false, spacing: "small" }, - { type: "text", text: `-# ID: ${params.resolved.id}` }, - ], - }; -} - const discordMessageActions: ChannelMessageActionAdapter = { describeMessageTool: (ctx) => getDiscordRuntime().channel.discord.messageActions?.describeMessageTool?.(ctx) ?? null, @@ -506,49 +355,6 @@ export const discordPlugin: ChannelPlugin payload, }), }, - render: { - plugin: { - buildPendingPayload: ({ cfg, request, target, nowMs }) => { - const normalizedChannel = normalizeMessageChannel(target.channel) ?? target.channel; - const interactiveEnabled = - normalizedChannel === "discord" && - isDiscordExecApprovalClientEnabled({ cfg, accountId: target.accountId }); - return buildPluginApprovalPendingReplyPayload({ - request, - nowMs, - text: formatDiscordApprovalPreview( - buildPluginApprovalRequestMessage(request, nowMs), - 10_000, - ), - approvalSlug: request.id.slice(0, 8), - channelData: interactiveEnabled - ? { - discord: { - components: buildDiscordPluginPendingComponentSpec({ request }), - }, - } - : undefined, - }); - }, - buildResolvedPayload: ({ resolved }) => { - const componentSpec = buildDiscordPluginResolvedComponentSpec({ resolved }); - return buildPluginApprovalResolvedReplyPayload({ - resolved, - text: formatDiscordApprovalPreview( - buildPluginApprovalResolvedMessage(resolved), - 10_000, - ), - channelData: componentSpec - ? { - discord: { - components: componentSpec, - }, - } - : undefined, - }); - }, - }, - }, }, directory: createChannelDirectoryAdapter({ listPeers: async (params) => listDiscordDirectoryPeersFromConfig(params), diff --git a/extensions/telegram/src/button-types.ts b/extensions/telegram/src/button-types.ts index 9aaaf55e655..28af123b363 100644 --- a/extensions/telegram/src/button-types.ts +++ b/extensions/telegram/src/button-types.ts @@ -16,6 +16,11 @@ export type TelegramInlineButton = { export type TelegramInlineButtons = ReadonlyArray>; const TELEGRAM_INTERACTIVE_ROW_SIZE = 3; +const MAX_CALLBACK_DATA_BYTES = 64; + +function fitsTelegramCallbackData(value: string): boolean { + return Buffer.byteLength(value, "utf8") <= MAX_CALLBACK_DATA_BYTES; +} function toTelegramButtonStyle( style?: InteractiveReplyButton["style"], @@ -28,11 +33,14 @@ function chunkInteractiveButtons( rows: TelegramInlineButton[][], ) { for (let i = 0; i < buttons.length; i += TELEGRAM_INTERACTIVE_ROW_SIZE) { - const row = buttons.slice(i, i + TELEGRAM_INTERACTIVE_ROW_SIZE).map((button) => ({ - text: button.label, - callback_data: button.value, - style: toTelegramButtonStyle(button.style), - })); + const row = buttons + .slice(i, i + TELEGRAM_INTERACTIVE_ROW_SIZE) + .filter((button) => fitsTelegramCallbackData(button.value)) + .map((button) => ({ + text: button.label, + callback_data: button.value, + style: toTelegramButtonStyle(button.style), + })); if (row.length > 0) { rows.push(row); } diff --git a/extensions/telegram/src/channel.test.ts b/extensions/telegram/src/channel.test.ts index c2f35d54cfb..b105bce01d2 100644 --- a/extensions/telegram/src/channel.test.ts +++ b/extensions/telegram/src/channel.test.ts @@ -1,5 +1,4 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; -import type { PluginApprovalRequest } from "openclaw/plugin-sdk/infra-runtime"; import type { PluginRuntime } from "openclaw/plugin-sdk/testing"; import { afterEach, describe, expect, it, vi } from "vitest"; import { createStartAccountContext } from "../../../test/helpers/plugins/start-account-context.js"; @@ -162,26 +161,6 @@ function installSendMessageRuntime( return sendMessageTelegram; } -function createPluginApprovalRequest( - overrides: Partial = {}, -): PluginApprovalRequest { - return { - id: "plugin:12345678-1234-1234-1234-1234567890ab", - request: { - title: "Sensitive plugin action", - description: "The plugin requested a sensitive operation.", - severity: "warning", - toolName: "plugin.tool", - pluginId: "plugin-test", - agentId: "agent-main", - sessionKey: "agent:agent-main:telegram:12345", - ...overrides, - }, - createdAtMs: 1_000, - expiresAtMs: 61_000, - }; -} - afterEach(() => { clearTelegramRuntime(); vi.clearAllMocks(); @@ -580,34 +559,6 @@ describe("telegramPlugin duplicate token guard", () => { expect(result).toMatchObject({ channel: "telegram", messageId: "tg-4" }); }); - it("builds plugin approval pending payload with callback ids that preserve allow-always", () => { - const request = createPluginApprovalRequest(); - const payload = telegramPlugin.execApprovals?.render?.plugin?.buildPendingPayload?.({ - cfg: createCfg(), - request, - target: { channel: "telegram", to: "12345" }, - nowMs: 2_000, - }); - - expect(payload?.text).toContain("Plugin approval required"); - const channelData = payload?.channelData as - | { - execApproval?: { approvalId?: string; approvalSlug?: string }; - telegram?: { buttons?: Array> }; - } - | undefined; - expect(channelData?.execApproval?.approvalId).toBe(request.id); - expect(channelData?.execApproval?.approvalSlug).toBe(request.id); - const buttons = channelData?.telegram?.buttons; - expect(buttons).toBeDefined(); - expect(buttons?.[0]?.some((button) => button.text === "Allow Always")).toBe(true); - for (const row of buttons ?? []) { - for (const button of row) { - expect(Buffer.byteLength(button.callback_data, "utf8")).toBeLessThanOrEqual(64); - } - } - }); - it("ignores accounts with missing tokens during duplicate-token checks", async () => { const cfg = createCfg(); cfg.channels!.telegram!.accounts!.ops = {} as never; @@ -721,7 +672,7 @@ describe("telegramPlugin duplicate token guard", () => { token: undefined as unknown as string, } as ResolvedTelegramAccount; - await expect(telegramPlugin.gateway!.startAccount!(ctx)).resolves.toBeUndefined(); + await telegramPlugin.gateway!.startAccount!(ctx); expect(probeTelegramMock).toHaveBeenCalledWith("", 2500, { accountId: "ops", proxyUrl: undefined, diff --git a/extensions/telegram/src/channel.ts b/extensions/telegram/src/channel.ts index fd16145b5dd..ca48f34e1d0 100644 --- a/extensions/telegram/src/channel.ts +++ b/extensions/telegram/src/channel.ts @@ -2,11 +2,7 @@ import { buildDmGroupAccountAllowlistAdapter, createNestedAllowlistOverrideResolver, } from "openclaw/plugin-sdk/allowlist-config-edit"; -import { - buildPluginApprovalPendingReplyPayload, - buildPluginApprovalRequestMessage, - createApproverRestrictedNativeApprovalAdapter, -} from "openclaw/plugin-sdk/approval-runtime"; +import { createApproverRestrictedNativeApprovalAdapter } from "openclaw/plugin-sdk/approval-runtime"; import { createPairingPrefixStripper } from "openclaw/plugin-sdk/channel-pairing"; import { createAllowlistProviderRouteAllowlistWarningCollector } from "openclaw/plugin-sdk/channel-policy"; import { attachChannelToResult } from "openclaw/plugin-sdk/channel-send-result"; @@ -44,14 +40,12 @@ import { type ResolvedTelegramAccount, } from "./accounts.js"; import { resolveTelegramAutoThreadId } from "./action-threading.js"; -import { buildTelegramExecApprovalButtons } from "./approval-buttons.js"; import * as auditModule from "./audit.js"; import { buildTelegramGroupPeerId } from "./bot/helpers.js"; import { listTelegramDirectoryGroupsFromConfig, listTelegramDirectoryPeersFromConfig, } from "./directory-config.js"; -import { buildTelegramExecApprovalPendingPayload } from "./exec-approval-forwarding.js"; import { getTelegramExecApprovalApprovers, isTelegramExecApprovalApprover, @@ -495,30 +489,6 @@ export const telegramPlugin = createChatChannelPlugin({ }).catch(() => {}); }, }, - render: { - exec: { - buildPendingPayload: ({ request, nowMs }) => - buildTelegramExecApprovalPendingPayload({ request, nowMs }), - }, - plugin: { - buildPendingPayload: ({ request, nowMs }) => { - const buttons = buildTelegramExecApprovalButtons(request.id); - return buildPluginApprovalPendingReplyPayload({ - request, - nowMs, - text: buildPluginApprovalRequestMessage(request, nowMs), - approvalSlug: request.id, - channelData: buttons - ? { - telegram: { - buttons, - }, - } - : undefined, - }); - }, - }, - }, }, directory: createChannelDirectoryAdapter({ listPeers: async (params) => listTelegramDirectoryPeersFromConfig(params), diff --git a/extensions/telegram/src/exec-approval-forwarding.ts b/extensions/telegram/src/exec-approval-forwarding.ts index f0a02030a43..60a5893a3a8 100644 --- a/extensions/telegram/src/exec-approval-forwarding.ts +++ b/extensions/telegram/src/exec-approval-forwarding.ts @@ -5,7 +5,6 @@ import { } from "openclaw/plugin-sdk/approval-runtime"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import { normalizeMessageChannel } from "openclaw/plugin-sdk/routing"; -import { buildTelegramExecApprovalButtons } from "./approval-buttons.js"; import { isTelegramExecApprovalClientEnabled } from "./exec-approvals.js"; export function shouldSuppressTelegramExecApprovalForwardingFallback(params: { @@ -30,7 +29,7 @@ export function buildTelegramExecApprovalPendingPayload(params: { request: ExecApprovalRequest; nowMs: number; }) { - const payload = buildExecApprovalPendingReplyPayload({ + return buildExecApprovalPendingReplyPayload({ approvalId: params.request.id, approvalSlug: params.request.id.slice(0, 8), approvalCommandId: params.request.id, @@ -41,15 +40,4 @@ export function buildTelegramExecApprovalPendingPayload(params: { expiresAtMs: params.request.expiresAtMs, nowMs: params.nowMs, }); - const buttons = buildTelegramExecApprovalButtons(params.request.id); - if (!buttons) { - return payload; - } - return { - ...payload, - channelData: { - ...payload.channelData, - telegram: { buttons }, - }, - }; } diff --git a/extensions/telegram/src/inline-buttons.test.ts b/extensions/telegram/src/inline-buttons.test.ts index c24c767598f..f5b53982853 100644 --- a/extensions/telegram/src/inline-buttons.test.ts +++ b/extensions/telegram/src/inline-buttons.test.ts @@ -84,6 +84,22 @@ describe("buildTelegramInteractiveButtons", () => { [{ text: "Alpha", callback_data: "alpha", style: undefined }], ]); }); + + it("drops shared buttons whose callback data exceeds Telegram's limit", () => { + expect( + buildTelegramInteractiveButtons({ + blocks: [ + { + type: "buttons", + buttons: [ + { label: "Keep", value: "keep" }, + { label: "Too long", value: `a${"b".repeat(64)}` }, + ], + }, + ], + }), + ).toEqual([[{ text: "Keep", callback_data: "keep", style: undefined }]]); + }); }); describe("resolveTelegramInlineButtons", () => { diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts index 366077bd631..309dba1736a 100644 --- a/src/infra/exec-approval-forwarder.test.ts +++ b/src/infra/exec-approval-forwarder.test.ts @@ -1,10 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { ChannelPlugin } from "../channels/plugins/types.js"; import type { OpenClawConfig } from "../config/config.js"; -import { - buildTelegramExecApprovalPendingPayload, - shouldSuppressTelegramExecApprovalForwardingFallback, -} from "../plugin-sdk/telegram.js"; +import { shouldSuppressTelegramExecApprovalForwardingFallback } from "../plugin-sdk/telegram.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; import { createChannelTestPluginBase, createTestRegistry } from "../test-utils/channel-plugins.js"; import { createExecApprovalForwarder } from "./exec-approval-forwarder.js"; @@ -56,12 +53,6 @@ const telegramApprovalPlugin: Pick< shouldSuppressForwardingFallback: (params) => shouldSuppressTelegramExecApprovalForwardingFallback(params), }, - render: { - exec: { - buildPendingPayload: ({ request, nowMs }) => - buildTelegramExecApprovalPendingPayload({ request, nowMs }), - }, - }, }, }; const discordApprovalPlugin: Pick< @@ -306,7 +297,7 @@ describe("exec approval forwarder", () => { expect(deliver).not.toHaveBeenCalled(); }); - it("attaches explicit telegram buttons in forwarded telegram fallback payloads", async () => { + it("attaches shared interactive approval buttons in forwarded fallback payloads", async () => { vi.useFakeTimers(); const { deliver, forwarder } = createForwarder({ cfg: makeTargetsCfg([{ channel: "telegram", to: "123" }]), @@ -334,15 +325,30 @@ describe("exec approval forwarder", () => { execApproval: expect.objectContaining({ approvalId: "req-1", }), - telegram: { - buttons: [ - [ - { text: "Allow Once", callback_data: "/approve req-1 allow-once" }, - { text: "Allow Always", callback_data: "/approve req-1 always" }, + }, + interactive: { + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Allow Once", + value: "/approve req-1 allow-once", + style: "success", + }, + { + label: "Allow Always", + value: "/approve req-1 always", + style: "primary", + }, + { + label: "Deny", + value: "/approve req-1 deny", + style: "danger", + }, ], - [{ text: "Deny", callback_data: "/approve req-1 deny" }], - ], - }, + }, + ], }, }), ], diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index 63d7e2c5ba7..335e47068bf 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/src/infra/exec-approval-forwarder.ts @@ -7,6 +7,10 @@ import type { ExecApprovalForwardTarget, } from "../config/types.approvals.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; +import { + buildApprovalPendingReplyPayload, + buildPluginApprovalPendingReplyPayload, +} from "../plugin-sdk/approval-renderers.js"; import { parseAgentSessionKey } from "../routing/session-key.js"; import { compileConfigRegex } from "../security/config-regex.js"; import { testRegexWithBoundedInput } from "../security/safe-regex.js"; @@ -284,7 +288,11 @@ function buildRequestPayloadForTarget( if (pluginPayload) { return pluginPayload; } - return { text: buildRequestMessage(request, nowMsValue) }; + return buildApprovalPendingReplyPayload({ + approvalId: request.id, + approvalSlug: request.id.slice(0, 8), + text: buildRequestMessage(request, nowMsValue), + }); } function buildResolvedPayloadForTarget( @@ -563,7 +571,14 @@ export function createExecApprovalForwarder( nowMs: nowMs(), }) : null; - return adapterPayload ?? { text: buildPluginApprovalRequestMessage(request, nowMs()) }; + return ( + adapterPayload ?? + buildPluginApprovalPendingReplyPayload({ + request, + nowMs: nowMs(), + text: buildPluginApprovalRequestMessage(request, nowMs()), + }) + ); }, beforeDeliver: async (target, payload) => { const channel = normalizeMessageChannel(target.channel) ?? target.channel; diff --git a/src/infra/exec-approval-reply.test.ts b/src/infra/exec-approval-reply.test.ts index 39a3e82e596..baaaba8200c 100644 --- a/src/infra/exec-approval-reply.test.ts +++ b/src/infra/exec-approval-reply.test.ts @@ -95,6 +95,30 @@ describe("exec approval reply helpers", () => { allowedDecisions: ["allow-once", "allow-always", "deny"], }, }); + expect(payload.interactive).toEqual({ + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Allow Once", + value: "/approve req-1 allow-once", + style: "success", + }, + { + label: "Allow Always", + value: "/approve req-1 always", + style: "primary", + }, + { + label: "Deny", + value: "/approve req-1 deny", + style: "danger", + }, + ], + }, + ], + }); expect(payload.text).toContain("Heads up."); expect(payload.text).toContain("```txt\n/approve slug-1 allow-once\n```"); expect(payload.text).toContain("```sh\necho ok\n```"); diff --git a/src/infra/exec-approval-reply.ts b/src/infra/exec-approval-reply.ts index 5c3c2667a00..4bbf77019fc 100644 --- a/src/infra/exec-approval-reply.ts +++ b/src/infra/exec-approval-reply.ts @@ -1,4 +1,5 @@ import type { ReplyPayload } from "../auto-reply/types.js"; +import type { InteractiveReply, InteractiveReplyButton } from "../interactive/payload.js"; import type { ExecHost } from "./exec-approvals.js"; export type ExecApprovalReplyDecision = "allow-once" | "allow-always" | "deny"; @@ -33,6 +34,55 @@ export type ExecApprovalUnavailableReplyParams = { sentApproverDms?: boolean; }; +const DEFAULT_ALLOWED_DECISIONS = ["allow-once", "allow-always", "deny"] as const; + +function buildApprovalDecisionCommandValue(params: { + approvalId: string; + decision: ExecApprovalReplyDecision; +}): string { + return `/approve ${params.approvalId} ${params.decision === "allow-always" ? "always" : params.decision}`; +} + +function buildApprovalInteractiveButtons( + allowedDecisions: readonly ExecApprovalReplyDecision[], + approvalId: string, +): InteractiveReplyButton[] { + const buttons: InteractiveReplyButton[] = []; + if (allowedDecisions.includes("allow-once")) { + buttons.push({ + label: "Allow Once", + value: buildApprovalDecisionCommandValue({ approvalId, decision: "allow-once" }), + style: "success", + }); + } + if (allowedDecisions.includes("allow-always")) { + buttons.push({ + label: "Allow Always", + value: buildApprovalDecisionCommandValue({ approvalId, decision: "allow-always" }), + style: "primary", + }); + } + if (allowedDecisions.includes("deny")) { + buttons.push({ + label: "Deny", + value: buildApprovalDecisionCommandValue({ approvalId, decision: "deny" }), + style: "danger", + }); + } + return buttons; +} + +export function buildApprovalInteractiveReply(params: { + approvalId: string; + allowedDecisions?: readonly ExecApprovalReplyDecision[]; +}): InteractiveReply | undefined { + const buttons = buildApprovalInteractiveButtons( + params.allowedDecisions ?? DEFAULT_ALLOWED_DECISIONS, + params.approvalId, + ); + return buttons.length > 0 ? { blocks: [{ type: "buttons", buttons }] } : undefined; +} + export function getExecApprovalApproverDmNoticeText(): string { return "Approval required. I sent approval DMs to the approvers for this account."; } @@ -137,11 +187,12 @@ export function buildExecApprovalPendingReplyPayload( return { text: lines.join("\n\n"), + interactive: buildApprovalInteractiveReply({ approvalId: params.approvalId }), channelData: { execApproval: { approvalId: params.approvalId, approvalSlug: params.approvalSlug, - allowedDecisions: ["allow-once", "allow-always", "deny"], + allowedDecisions: DEFAULT_ALLOWED_DECISIONS, }, }, }; diff --git a/src/infra/plugin-approval-forwarder.test.ts b/src/infra/plugin-approval-forwarder.test.ts index 2352b657e1a..9cb70dca7ed 100644 --- a/src/infra/plugin-approval-forwarder.test.ts +++ b/src/infra/plugin-approval-forwarder.test.ts @@ -83,13 +83,38 @@ describe("plugin approval forwarding", () => { expect(deliver).toHaveBeenCalled(); }); const deliveryArgs = deliver.mock.calls[0]?.[0] as - | { payloads?: Array<{ text?: string }> } + | { payloads?: Array<{ text?: string; interactive?: unknown }> } | undefined; - const text = deliveryArgs?.payloads?.[0]?.text ?? ""; + const payload = deliveryArgs?.payloads?.[0]; + const text = payload?.text ?? ""; expect(text).toContain("Plugin approval required"); expect(text).toContain("Sensitive tool call"); expect(text).toContain("plugin-req-1"); expect(text).toContain("/approve"); + expect(payload?.interactive).toEqual({ + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Allow Once", + value: "/approve plugin-req-1 allow-once", + style: "success", + }, + { + label: "Allow Always", + value: "/approve plugin-req-1 always", + style: "primary", + }, + { + label: "Deny", + value: "/approve plugin-req-1 deny", + style: "danger", + }, + ], + }, + ], + }); }); it("includes severity icon for critical", async () => { @@ -180,19 +205,6 @@ describe("plugin approval forwarding", () => { expect(deliveryArgs?.payloads?.[0]?.text).toBe("custom adapter payload"); }); - it("falls back to plugin text when no adapter exists", async () => { - const deliver = vi.fn().mockResolvedValue([]); - const { forwarder } = createForwarder({ cfg: PLUGIN_TARGETS_CFG, deliver }); - await forwarder.handlePluginApprovalRequested!(makePluginRequest()); - await vi.waitFor(() => { - expect(deliver).toHaveBeenCalled(); - }); - const text = - (deliver.mock.calls[0]?.[0] as { payloads?: Array<{ text?: string }> })?.payloads?.[0] - ?.text ?? ""; - expect(text).toContain("Plugin approval required"); - }); - it("calls beforeDeliverPending before plugin approval delivery", async () => { const beforeDeliverPending = vi.fn(); const adapterPlugin: Pick< diff --git a/src/plugin-sdk/approval-renderers.test.ts b/src/plugin-sdk/approval-renderers.test.ts index c9de452d426..0abbe604846 100644 --- a/src/plugin-sdk/approval-renderers.test.ts +++ b/src/plugin-sdk/approval-renderers.test.ts @@ -1,10 +1,45 @@ import { describe, expect, it } from "vitest"; import { + buildApprovalPendingReplyPayload, buildPluginApprovalPendingReplyPayload, buildPluginApprovalResolvedReplyPayload, } from "./approval-renderers.js"; describe("plugin-sdk/approval-renderers", () => { + it("builds shared approval payloads with generic interactive commands", () => { + const payload = buildApprovalPendingReplyPayload({ + approvalId: "plugin:approval-123", + approvalSlug: "plugin:a", + text: "Approval required @everyone", + }); + + expect(payload.text).toContain("@\u200beveryone"); + expect(payload.interactive).toEqual({ + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Allow Once", + value: "/approve plugin:approval-123 allow-once", + style: "success", + }, + { + label: "Allow Always", + value: "/approve plugin:approval-123 always", + style: "primary", + }, + { + label: "Deny", + value: "/approve plugin:approval-123 deny", + style: "danger", + }, + ], + }, + ], + }); + }); + it("builds plugin pending payloads with approval metadata and extra channel data", () => { const payload = buildPluginApprovalPendingReplyPayload({ request: { @@ -20,12 +55,36 @@ describe("plugin-sdk/approval-renderers", () => { approvalSlug: "custom-slug", channelData: { telegram: { - buttons: [[{ text: "Allow Once", callback_data: "/approve id allow-once" }]], + quoteText: "quoted", }, }, }); expect(payload.text).toContain("Plugin approval required"); + expect(payload.interactive).toEqual({ + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Allow Once", + value: "/approve plugin-approval-123 allow-once", + style: "success", + }, + { + label: "Allow Always", + value: "/approve plugin-approval-123 always", + style: "primary", + }, + { + label: "Deny", + value: "/approve plugin-approval-123 deny", + style: "danger", + }, + ], + }, + ], + }); expect(payload.channelData).toMatchObject({ execApproval: { approvalId: "plugin-approval-123", @@ -33,7 +92,7 @@ describe("plugin-sdk/approval-renderers", () => { allowedDecisions: ["allow-once", "allow-always", "deny"], }, telegram: { - buttons: [[{ text: "Allow Once", callback_data: "/approve id allow-once" }]], + quoteText: "quoted", }, }); }); diff --git a/src/plugin-sdk/approval-renderers.ts b/src/plugin-sdk/approval-renderers.ts index a9a54c3b13e..9aef26d7ee4 100644 --- a/src/plugin-sdk/approval-renderers.ts +++ b/src/plugin-sdk/approval-renderers.ts @@ -1,5 +1,8 @@ import type { ReplyPayload } from "../auto-reply/types.js"; -import type { ExecApprovalReplyDecision } from "../infra/exec-approval-reply.js"; +import { + buildApprovalInteractiveReply, + type ExecApprovalReplyDecision, +} from "../infra/exec-approval-reply.js"; import { buildPluginApprovalRequestMessage, buildPluginApprovalResolvedMessage, @@ -9,6 +12,39 @@ import { const DEFAULT_ALLOWED_DECISIONS = ["allow-once", "allow-always", "deny"] as const; +function neutralizeApprovalText(value: string): string { + return value + .replace(/@everyone/gi, "@\u200beveryone") + .replace(/@here/gi, "@\u200bhere") + .replace(/<@/g, "<@\u200b") + .replace(/<#/g, "<#\u200b"); +} + +export function buildApprovalPendingReplyPayload(params: { + approvalId: string; + approvalSlug: string; + text: string; + allowedDecisions?: readonly ExecApprovalReplyDecision[]; + channelData?: Record; +}): ReplyPayload { + const allowedDecisions = params.allowedDecisions ?? DEFAULT_ALLOWED_DECISIONS; + return { + text: neutralizeApprovalText(params.text), + interactive: buildApprovalInteractiveReply({ + approvalId: params.approvalId, + allowedDecisions, + }), + channelData: { + execApproval: { + approvalId: params.approvalId, + approvalSlug: params.approvalSlug, + allowedDecisions, + }, + ...params.channelData, + }, + }; +} + export function buildPluginApprovalPendingReplyPayload(params: { request: PluginApprovalRequest; nowMs: number; @@ -17,17 +53,13 @@ export function buildPluginApprovalPendingReplyPayload(params: { allowedDecisions?: readonly ExecApprovalReplyDecision[]; channelData?: Record; }): ReplyPayload { - return { + return buildApprovalPendingReplyPayload({ + approvalId: params.request.id, + approvalSlug: params.approvalSlug ?? params.request.id.slice(0, 8), text: params.text ?? buildPluginApprovalRequestMessage(params.request, params.nowMs), - channelData: { - execApproval: { - approvalId: params.request.id, - approvalSlug: params.approvalSlug ?? params.request.id.slice(0, 8), - allowedDecisions: params.allowedDecisions ?? DEFAULT_ALLOWED_DECISIONS, - }, - ...params.channelData, - }, - }; + allowedDecisions: params.allowedDecisions, + channelData: params.channelData, + }); } export function buildPluginApprovalResolvedReplyPayload(params: { @@ -37,10 +69,14 @@ export function buildPluginApprovalResolvedReplyPayload(params: { }): ReplyPayload { return params.channelData ? { - text: params.text ?? buildPluginApprovalResolvedMessage(params.resolved), + text: neutralizeApprovalText( + params.text ?? buildPluginApprovalResolvedMessage(params.resolved), + ), channelData: params.channelData, } : { - text: params.text ?? buildPluginApprovalResolvedMessage(params.resolved), + text: neutralizeApprovalText( + params.text ?? buildPluginApprovalResolvedMessage(params.resolved), + ), }; } diff --git a/src/plugin-sdk/approval-runtime.ts b/src/plugin-sdk/approval-runtime.ts index 491f870197a..7df740af572 100644 --- a/src/plugin-sdk/approval-runtime.ts +++ b/src/plugin-sdk/approval-runtime.ts @@ -33,6 +33,7 @@ export { } from "../infra/plugin-approvals.js"; export { createApproverRestrictedNativeApprovalAdapter } from "./approval-delivery-helpers.js"; export { + buildApprovalPendingReplyPayload, buildPluginApprovalPendingReplyPayload, buildPluginApprovalResolvedReplyPayload, } from "./approval-renderers.js";