diff --git a/CHANGELOG.md b/CHANGELOG.md index da176b4b55f..a62821d84f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Slack/exec approvals: resolve native approval button clicks over the Gateway instead of delivering `/approve ...` as plain agent text, preserving retry buttons if Gateway resolution fails. Fixes #71023. (#71025) Thanks @marusan03. - Slack/files: return non-image `download-file` results as local file paths instead of image payloads, and include Slack file IDs in inbound file placeholders so agents can call `download-file`. Fixes #71212. Thanks @teamrazo. - Discord/replies: run `message_sending` plugin hooks for Discord reply delivery, including DM targets, so plugins can transform or cancel outbound Discord replies consistently with other channels. Fixes #59350. (#71094) Thanks @wei840222. - Control UI/commands: carry provider-owned thinking option ids/labels in session rows and defaults so fresh sessions show and accept dynamic modes such as `adaptive`, `xhigh`, and `max`. Fixes #71269. Thanks @Young-Khalil. diff --git a/extensions/slack/src/monitor/events/interactions.block-actions.ts b/extensions/slack/src/monitor/events/interactions.block-actions.ts index 9442967e86d..a4d800cd6ee 100644 --- a/extensions/slack/src/monitor/events/interactions.block-actions.ts +++ b/extensions/slack/src/monitor/events/interactions.block-actions.ts @@ -1,7 +1,15 @@ import type { SlackActionMiddlewareArgs } from "@slack/bolt"; import type { Block, KnownBlock } from "@slack/web-api"; -import { enqueueSystemEvent } from "openclaw/plugin-sdk/infra-runtime"; +import { resolveApprovalOverGateway } from "openclaw/plugin-sdk/approval-gateway-runtime"; +import { + enqueueSystemEvent, + parseExecApprovalCommandText, +} from "openclaw/plugin-sdk/infra-runtime"; import { normalizeOptionalString } from "openclaw/plugin-sdk/text-runtime"; +import { + isSlackExecApprovalApprover, + isSlackExecApprovalAuthorizedSender, +} from "../../exec-approvals.js"; import { dispatchSlackPluginInteractiveHandler } from "../../interactive-dispatch.js"; import { SLACK_REPLY_BUTTON_ACTION_ID, @@ -521,6 +529,68 @@ async function handleSlackPluginBindingApproval(params: { return true; } +async function handleSlackExecApprovalInteraction(params: { + ctx: SlackMonitorContext; + parsed: ParsedSlackBlockAction; + pluginInteractionData: string; + respond?: SlackBlockActionRespond; +}): Promise { + const approval = parseExecApprovalCommandText(params.pluginInteractionData); + if (!approval) { + return false; + } + const pluginApprovalAuthorizedSender = isSlackExecApprovalApprover({ + cfg: params.ctx.cfg, + accountId: params.ctx.accountId, + senderId: params.parsed.userId, + }); + const execApprovalAuthorizedSender = isSlackExecApprovalAuthorizedSender({ + cfg: params.ctx.cfg, + accountId: params.ctx.accountId, + senderId: params.parsed.userId, + }); + const isPluginApproval = approval.approvalId.startsWith("plugin:"); + const authorized = isPluginApproval + ? pluginApprovalAuthorizedSender + : execApprovalAuthorizedSender || pluginApprovalAuthorizedSender; + if (!authorized) { + params.ctx.runtime.log?.( + `slack:interaction drop exec approval user=${params.parsed.userId} (not authorized)`, + ); + await respondEphemeral(params.respond, "You are not authorized to approve this request."); + return true; + } + + try { + await resolveApprovalOverGateway({ + cfg: params.ctx.cfg, + approvalId: approval.approvalId, + decision: approval.decision, + senderId: params.parsed.userId, + allowPluginFallback: pluginApprovalAuthorizedSender, + clientDisplayName: `Slack approval (${params.parsed.userId.trim() || "unknown"})`, + }); + } catch (error) { + params.ctx.runtime.log?.( + `slack:interaction exec approval resolve failed id=${approval.approvalId}: ${String(error)}`, + ); + throw error; + } + + try { + await updateSlackInteractionMessage({ + ctx: params.ctx, + channelId: params.parsed.channelId, + messageTs: params.parsed.messageTs, + text: params.parsed.typedBody.message?.text ?? "", + blocks: [], + }); + } catch { + // Best-effort cleanup only. + } + return true; +} + async function dispatchSlackPluginInteraction(params: { ctx: SlackMonitorContext; parsed: ParsedSlackBlockAction; @@ -742,6 +812,21 @@ async function handleSlackBlockAction(params: { return; } params.trackEvent?.(); + const pluginInteractionData = buildSlackPluginInteractionData({ + actionId: parsed.actionId, + summary: parsed.actionSummary, + }); + if (pluginInteractionData && isSlackReplyActionId(parsed.actionId)) { + const handledExecApproval = await handleSlackExecApprovalInteraction({ + ctx: params.ctx, + parsed, + pluginInteractionData, + respond, + }); + if (handledExecApproval) { + return; + } + } const auth = await authorizeSlackBlockAction({ ctx: params.ctx, parsed, @@ -750,10 +835,6 @@ async function handleSlackBlockAction(params: { if (!auth.allowed) { return; } - const pluginInteractionData = buildSlackPluginInteractionData({ - actionId: parsed.actionId, - summary: parsed.actionSummary, - }); if (pluginInteractionData && isSlackReplyActionId(parsed.actionId)) { const handledBindingApproval = await handleSlackPluginBindingApproval({ ctx: params.ctx, diff --git a/extensions/slack/src/monitor/events/interactions.test.ts b/extensions/slack/src/monitor/events/interactions.test.ts index dcb122aefe5..637bf3f357f 100644 --- a/extensions/slack/src/monitor/events/interactions.test.ts +++ b/extensions/slack/src/monitor/events/interactions.test.ts @@ -10,11 +10,22 @@ const dispatchPluginInteractiveHandlerMock = vi.hoisted(() => ); const resolvePluginConversationBindingApprovalMock = vi.hoisted(() => vi.fn()); const buildPluginBindingResolvedTextMock = vi.hoisted(() => vi.fn(() => "Binding updated.")); +const resolveApprovalOverGatewayMock = vi.hoisted(() => + vi.fn<(arg: unknown) => Promise>(async () => undefined), +); let registerSlackInteractionEvents: typeof import("./interactions.js").registerSlackInteractionEvents; -vi.mock("openclaw/plugin-sdk/infra-runtime", () => ({ - enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args), +vi.mock("openclaw/plugin-sdk/infra-runtime", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args), + }; +}); + +vi.mock("openclaw/plugin-sdk/approval-gateway-runtime", () => ({ + resolveApprovalOverGateway: (arg: unknown) => resolveApprovalOverGatewayMock(arg), })); vi.mock("openclaw/plugin-sdk/security-runtime", () => ({ @@ -161,6 +172,7 @@ function createContext(overrides?: { allowFrom?: string[]; allowNameMatching?: boolean; channelsConfig?: Record; + cfg?: Record; shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; isChannelAllowed?: (params: { channelId?: string; @@ -221,6 +233,17 @@ function createContext(overrides?: { const ctx = { app, accountId: "default", + cfg: overrides?.cfg ?? { + channels: { + slack: { + execApprovals: { + enabled: true, + approvers: ["U123"], + target: "both", + }, + }, + }, + }, runtime: { log: runtimeLog }, dmEnabled: overrides?.dmEnabled ?? true, dmPolicy: overrides?.dmPolicy ?? ("open" as const), @@ -263,6 +286,8 @@ describe("registerSlackInteractionEvents", () => { resolvePluginConversationBindingApprovalMock.mockResolvedValue({ status: "expired" }); buildPluginBindingResolvedTextMock.mockClear(); buildPluginBindingResolvedTextMock.mockReturnValue("Binding updated."); + resolveApprovalOverGatewayMock.mockClear(); + resolveApprovalOverGatewayMock.mockResolvedValue(undefined); dispatchPluginInteractiveHandlerMock.mockResolvedValue({ matched: false, handled: false, @@ -644,6 +669,168 @@ describe("registerSlackInteractionEvents", () => { expect(enqueueSystemEventMock).not.toHaveBeenCalled(); }); + it("resolves exec approvals from shared interactive Slack actions", async () => { + const { ctx, app, getHandler } = createContext({ allowFrom: ["U999"] }); + registerSlackInteractionEvents({ ctx: ctx as never }); + + const handler = getHandler(); + expect(handler).toBeTruthy(); + + const ack = vi.fn().mockResolvedValue(undefined); + const respond = vi.fn().mockResolvedValue(undefined); + await handler!({ + ack, + respond, + body: { + user: { id: "U123" }, + channel: { id: "C1" }, + container: { channel_id: "C1", message_ts: "100.200", thread_ts: "100.100" }, + message: { + ts: "100.200", + text: "Exec approval required", + blocks: [ + { + type: "actions", + block_id: "exec_actions", + elements: [{ type: "button", action_id: "openclaw:reply_button" }], + }, + ], + }, + }, + action: { + type: "button", + action_id: "openclaw:reply_button", + block_id: "exec_actions", + value: "/approve req-123 allow-once", + text: { type: "plain_text", text: "Allow once" }, + }, + }); + + expect(ack).toHaveBeenCalled(); + expect(resolveApprovalOverGatewayMock).toHaveBeenCalledWith({ + cfg: ctx.cfg, + approvalId: "req-123", + decision: "allow-once", + senderId: "U123", + allowPluginFallback: true, + clientDisplayName: "Slack approval (U123)", + }); + expect(resolvePluginConversationBindingApprovalMock).not.toHaveBeenCalled(); + expect(dispatchPluginInteractiveHandlerMock).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(app.client.chat.update).toHaveBeenCalledWith( + expect.objectContaining({ + channel: "C1", + ts: "100.200", + text: "Exec approval required", + blocks: [], + }), + ); + expect(respond).not.toHaveBeenCalled(); + }); + + it("keeps exec approval buttons when gateway resolution fails", async () => { + resolveApprovalOverGatewayMock.mockRejectedValueOnce(new Error("gateway down")); + const { ctx, app, getHandler } = createContext(); + registerSlackInteractionEvents({ ctx: ctx as never }); + + const handler = getHandler(); + expect(handler).toBeTruthy(); + + const ack = vi.fn().mockResolvedValue(undefined); + await expect( + handler!({ + ack, + body: { + user: { id: "U123" }, + channel: { id: "C1" }, + container: { channel_id: "C1", message_ts: "100.200" }, + message: { + ts: "100.200", + text: "Exec approval required", + blocks: [ + { + type: "actions", + block_id: "exec_actions", + elements: [{ type: "button", action_id: "openclaw:reply_button" }], + }, + ], + }, + }, + action: { + type: "button", + action_id: "openclaw:reply_button", + block_id: "exec_actions", + value: "/approve req-123 allow-once", + text: { type: "plain_text", text: "Allow once" }, + }, + }), + ).rejects.toThrow("gateway down"); + + expect(ack).toHaveBeenCalled(); + expect(resolveApprovalOverGatewayMock).toHaveBeenCalledTimes(1); + expect(app.client.chat.update).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); + + it("rejects unauthorized exec approval interactions without enqueueing them", async () => { + const { ctx, app, getHandler } = createContext({ + cfg: { + channels: { + slack: { + execApprovals: { + enabled: true, + approvers: ["U999"], + target: "both", + }, + }, + }, + }, + }); + registerSlackInteractionEvents({ ctx: ctx as never }); + + const handler = getHandler(); + expect(handler).toBeTruthy(); + + const ack = vi.fn().mockResolvedValue(undefined); + const respond = vi.fn().mockResolvedValue(undefined); + await handler!({ + ack, + respond, + body: { + user: { id: "U123" }, + channel: { id: "C1" }, + container: { channel_id: "C1", message_ts: "100.200" }, + message: { + ts: "100.200", + text: "Exec approval required", + blocks: [ + { + type: "actions", + block_id: "exec_actions", + elements: [{ type: "button", action_id: "openclaw:reply_button" }], + }, + ], + }, + }, + action: { + type: "button", + action_id: "openclaw:reply_button", + block_id: "exec_actions", + value: "/approve req-123 allow-once", + text: { type: "plain_text", text: "Allow once" }, + }, + }); + + expect(resolveApprovalOverGatewayMock).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(app.client.chat.update).not.toHaveBeenCalled(); + expect(respond).toHaveBeenCalledWith({ + text: "You are not authorized to approve this request.", + response_type: "ephemeral", + }); + }); + it("drops block actions when mismatch guard triggers", async () => { enqueueSystemEventMock.mockClear(); const { ctx, app, getHandler } = createContext({