From eab2b8fdca5c7956a0f401fc7be7812d620e2af0 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 16 Aug 2026 14:53:26 -0700 Subject: [PATCH] refactor(slack): mark approval headers with typed block ids (#124841) --- extensions/imessage/src/approval-native.ts | 4 +- .../imessage/src/approval-reactions.test.ts | 8 ++-- extensions/imessage/src/approval-reactions.ts | 17 +------ extensions/imessage/src/send.ts | 4 +- .../signal/src/approval-reactions.test.ts | 47 +------------------ extensions/signal/src/approval-reactions.ts | 16 +------ extensions/slack/src/approval-actions.ts | 1 + .../src/approval-handler.runtime.test.ts | 6 +++ .../slack/src/approval-handler.runtime.ts | 3 ++ .../events/interactions.block-actions.ts | 12 +++-- .../src/monitor/events/interactions.test.ts | 3 +- 11 files changed, 32 insertions(+), 89 deletions(-) diff --git a/extensions/imessage/src/approval-native.ts b/extensions/imessage/src/approval-native.ts index 051c0685dc91..a5b9c2586cac 100644 --- a/extensions/imessage/src/approval-native.ts +++ b/extensions/imessage/src/approval-native.ts @@ -3,6 +3,7 @@ import { createApproverRestrictedNativeApprovalCapabilityFromForwardingRoutes } import { createLazyChannelApprovalNativeRuntimeAdapter } from "openclaw/plugin-sdk/approval-handler-adapter-runtime"; import type { ChannelApprovalNativeRuntimeAdapter } from "openclaw/plugin-sdk/approval-handler-runtime"; import { shouldSuppressLocalNativeExecApprovalPrompt } from "openclaw/plugin-sdk/approval-native-runtime"; +import { addApprovalReactionHintToText } from "openclaw/plugin-sdk/approval-reaction-runtime"; import { buildTypedExecApprovalPendingReplyPayload, buildTypedPluginApprovalPendingReplyPayload, @@ -34,7 +35,6 @@ import { resolveIMessageAccount, } from "./accounts.js"; import { getIMessageApprovalApprovers, imessageApprovalAuth } from "./approval-auth.js"; -import { addIMessageApprovalReactionHintToText } from "./approval-reactions.js"; import { replaceApprovalIdPlaceholder } from "./approval-text.js"; import { normalizeIMessageMessagingTarget } from "./normalize.js"; import { inferIMessageTargetChatType } from "./targets.js"; @@ -204,7 +204,7 @@ function appendIMessageReactionHint(params: { text?: string; allowedDecisions: readonly ExecApprovalReplyDecision[]; }): string { - return addIMessageApprovalReactionHintToText({ + return addApprovalReactionHintToText({ text: params.text ?? "", allowedDecisions: params.allowedDecisions, }); diff --git a/extensions/imessage/src/approval-reactions.test.ts b/extensions/imessage/src/approval-reactions.test.ts index a9073d5d0974..f65e60914fe4 100644 --- a/extensions/imessage/src/approval-reactions.test.ts +++ b/extensions/imessage/src/approval-reactions.test.ts @@ -1,4 +1,5 @@ // Imessage tests cover approval reactions plugin behavior. +import { buildApprovalReactionHint } from "openclaw/plugin-sdk/approval-reaction-runtime"; import { buildTypedExecApprovalPendingReplyPayload } from "openclaw/plugin-sdk/approval-reply-runtime"; import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -6,7 +7,6 @@ import { listPendingIMessageApprovalReactionPollTargets } from "./approval-react import { addIMessageApprovalReactionHintToStructuredPayload, buildIMessageApprovalConversationKeyForTarget, - buildIMessageApprovalReactionHint, clearIMessageApprovalReactionTargetsForTest, handleIMessageApprovalReaction, maybeResolveIMessageApprovalReaction, @@ -78,9 +78,9 @@ describe("iMessage approval reactions", () => { }); it("renders shared reaction choices for allowed decisions", () => { - expect(buildIMessageApprovalReactionHint(["allow-once", "allow-always", "deny"])).toBe( - "React with:\n\nπŸ‘ Allow Once\n♾️ Allow Always\nπŸ‘Ž Deny", - ); + expect( + buildApprovalReactionHint({ allowedDecisions: ["allow-once", "allow-always", "deny"] }), + ).toBe("React with:\n\nπŸ‘ Allow Once\n♾️ Allow Always\nπŸ‘Ž Deny"); }); it("uses typed metadata to prepare shared forwarded prompts", () => { diff --git a/extensions/imessage/src/approval-reactions.ts b/extensions/imessage/src/approval-reactions.ts index 6c17c08a30a8..62c380fb3c0c 100644 --- a/extensions/imessage/src/approval-reactions.ts +++ b/extensions/imessage/src/approval-reactions.ts @@ -137,19 +137,6 @@ function listIMessageApprovalReactionBindings( return listApprovalReactionBindings({ allowedDecisions }); } -export function buildIMessageApprovalReactionHint( - allowedDecisions: readonly ExecApprovalReplyDecision[], -): string | null { - return buildApprovalReactionHint({ allowedDecisions }); -} - -export function addIMessageApprovalReactionHintToText(params: { - text: string; - allowedDecisions: readonly ExecApprovalReplyDecision[]; -}): string { - return addApprovalReactionHintToText(params); -} - type IMessageApprovalDeliveryBinding = ApprovalReactionDeliveryBinding & { approvalSlug: string; }; @@ -205,7 +192,7 @@ function visibleApprovalBindingMatches( if (!options.requireReactionHint) { return true; } - const hint = buildIMessageApprovalReactionHint(binding.allowedDecisions); + const hint = buildApprovalReactionHint({ allowedDecisions: binding.allowedDecisions }); return Boolean(hint && text.includes(hint)); } @@ -229,7 +216,7 @@ export function addIMessageApprovalReactionHintToStructuredPayload(params: { } return { ...params.payload, - text: addIMessageApprovalReactionHintToText({ + text: addApprovalReactionHintToText({ text, allowedDecisions: metadata.allowedDecisions, }), diff --git a/extensions/imessage/src/send.ts b/extensions/imessage/src/send.ts index 518a0f46bf3f..143b1c2b5fb2 100644 --- a/extensions/imessage/src/send.ts +++ b/extensions/imessage/src/send.ts @@ -1,6 +1,7 @@ // Imessage plugin module implements send behavior. import { constants, accessSync } from "node:fs"; import { basename } from "node:path"; +import { addApprovalReactionHintToText } from "openclaw/plugin-sdk/approval-reaction-runtime"; import type { ExecApprovalReplyDecision } from "openclaw/plugin-sdk/approval-reply-runtime"; import { createChannelPartialDeliveryError, @@ -39,7 +40,6 @@ import { type ResolvedIMessageAccount, } from "./accounts.js"; import { - addIMessageApprovalReactionHintToText, type IMessageApprovalConversationKey, registerIMessageApprovalReactionTarget, } from "./approval-reactions.js"; @@ -882,7 +882,7 @@ export async function sendMessageIMessage( ? account.config.mediaMaxMb * 1024 * 1024 : 16 * 1024 * 1024; let message = opts.approvalPrompt - ? addIMessageApprovalReactionHintToText({ + ? addApprovalReactionHintToText({ text, allowedDecisions: opts.approvalPrompt.allowedDecisions, }) diff --git a/extensions/signal/src/approval-reactions.test.ts b/extensions/signal/src/approval-reactions.test.ts index e4140a9e6352..d78e73570afc 100644 --- a/extensions/signal/src/approval-reactions.test.ts +++ b/extensions/signal/src/approval-reactions.test.ts @@ -1,3 +1,4 @@ +import { addApprovalReactionHintToText } from "openclaw/plugin-sdk/approval-reaction-runtime"; import { buildExecApprovalPendingReplyPayload, buildPluginApprovalPendingReplyPayload, @@ -5,9 +6,7 @@ import { // Signal tests cover approval reactions plugin behavior. import { beforeEach, describe, expect, it, vi } from "vitest"; import { - addSignalApprovalReactionHintToText, addSignalApprovalReactionHintToStructuredPayload, - buildSignalApprovalReactionHint, clearSignalApprovalReactionTargetsForTest, maybeResolveSignalApprovalReaction, registerSignalApprovalReactionTargetForDeliveredPayload, @@ -45,48 +44,6 @@ describe("Signal approval reactions", () => { resolverMocks.isApprovalNotFoundError.mockReturnValue(false); }); - it("renders thumbs-only reaction choices for allowed decisions", () => { - expect(buildSignalApprovalReactionHint(["allow-once", "deny"])).toBe( - "React with:\n\nπŸ‘ Allow Once\nπŸ‘Ž Deny", - ); - }); - - it("exposes allow-always as a reaction choice when allowed", () => { - expect(buildSignalApprovalReactionHint(["allow-once", "allow-always", "deny"])).toBe( - "React with:\n\nπŸ‘ Allow Once\n♾️ Allow Always\nπŸ‘Ž Deny", - ); - }); - - it("appends thumbs-only reaction choices to outbound approval prompts", () => { - expect( - addSignalApprovalReactionHintToText({ - text: "Exec approval required\nID: exec-1\n\nReply with: /approve exec-1 allow-once|deny", - allowedDecisions: ["allow-once", "deny"], - }), - ).toBe( - "Exec approval required\nID: exec-1\n\nReact with:\n\nπŸ‘ Allow Once\nπŸ‘Ž Deny\n\nReply with: /approve exec-1 allow-once|deny", - ); - }); - - it("does not duplicate reaction choices on native approval prompts", () => { - const prompt = [ - "Plugin approval required", - "Reply with: /approve plugin:abc allow-once|allow-always|deny", - "", - "React with:", - "", - "πŸ‘ Allow Once", - "πŸ‘Ž Deny", - ].join("\n"); - - expect( - addSignalApprovalReactionHintToText({ - text: prompt, - allowedDecisions: ["allow-once", "deny"], - }), - ).toBe(prompt); - }); - it("registers delivered structured approval payloads for reactions", async () => { const cfg = { channels: { @@ -472,7 +429,7 @@ describe("Signal approval reactions", () => { }); const deliveredPayload = { ...payload, - text: addSignalApprovalReactionHintToText({ + text: addApprovalReactionHintToText({ text: payload.text ?? "", allowedDecisions: ["allow-once", "deny"], }), diff --git a/extensions/signal/src/approval-reactions.ts b/extensions/signal/src/approval-reactions.ts index 4941c800a218..e8b388648604 100644 --- a/extensions/signal/src/approval-reactions.ts +++ b/extensions/signal/src/approval-reactions.ts @@ -3,7 +3,6 @@ import { matchesApprovalRequestFilters } from "openclaw/plugin-sdk/approval-clie import type { ApprovalResolveResult } from "openclaw/plugin-sdk/approval-gateway-runtime"; import { addApprovalReactionHintToText, - buildApprovalReactionHint, createApprovalReactionTargetStore, hasApprovalReactionHintText, listApprovalReactionBindings, @@ -355,19 +354,6 @@ function listSignalApprovalReactionBindings( return listApprovalReactionBindings({ allowedDecisions }); } -export function buildSignalApprovalReactionHint( - allowedDecisions: readonly ExecApprovalReplyDecision[], -): string | null { - return buildApprovalReactionHint({ allowedDecisions }); -} - -export function addSignalApprovalReactionHintToText(params: { - text: string; - allowedDecisions: readonly ExecApprovalReplyDecision[]; -}): string { - return addApprovalReactionHintToText(params); -} - function buildTargetRoute(params: { cfg: OpenClawConfig; accountId?: string | null; @@ -524,7 +510,7 @@ export function addSignalApprovalReactionHintToStructuredPayload(params: { } return { ...params.payload, - text: addSignalApprovalReactionHintToText({ + text: addApprovalReactionHintToText({ text: params.payload.text, allowedDecisions: metadata.allowedDecisions, }), diff --git a/extensions/slack/src/approval-actions.ts b/extensions/slack/src/approval-actions.ts index f73ee0681bb0..8860c1eb955c 100644 --- a/extensions/slack/src/approval-actions.ts +++ b/extensions/slack/src/approval-actions.ts @@ -4,6 +4,7 @@ import type { MessagePresentationAction } from "openclaw/plugin-sdk/interactive- import { SLACK_BUTTON_VALUE_MAX } from "./presentation.js"; const SLACK_APPROVAL_VALUE_PREFIX = "openclaw:approval:v1:"; +export const SLACK_APPROVAL_HEADER_BLOCK_ID = "openclaw_approval_header"; export type SlackApprovalAction = Extract; diff --git a/extensions/slack/src/approval-handler.runtime.test.ts b/extensions/slack/src/approval-handler.runtime.test.ts index bfe9d01b04af..f87ba47cfe44 100644 --- a/extensions/slack/src/approval-handler.runtime.test.ts +++ b/extensions/slack/src/approval-handler.runtime.test.ts @@ -350,6 +350,9 @@ describe("slackApprovalNativeRuntime", () => { }); expect(payload.text).toContain("*Exec approval required*"); + expect((payload.blocks as Array<{ block_id?: string }>)[0]?.block_id).toBe( + "openclaw_approval_header", + ); const actionsBlock = findSlackActionsBlock( payload.blocks as Array<{ type?: string; elements?: unknown[] }>, ); @@ -376,6 +379,9 @@ describe("slackApprovalNativeRuntime", () => { }); expect(payload.text).toContain("*Plugin approval required*"); + expect((payload.blocks as Array<{ block_id?: string }>)[0]?.block_id).toBe( + "openclaw_approval_header", + ); expect(payload.text).toContain("Share screen with Computer Use"); expect(payload.text).toContain("*Approval ID:* plugin:req-1"); expect(payload.text).not.toContain("*Command*"); diff --git a/extensions/slack/src/approval-handler.runtime.ts b/extensions/slack/src/approval-handler.runtime.ts index 3271180116f9..37df10d7e575 100644 --- a/extensions/slack/src/approval-handler.runtime.ts +++ b/extensions/slack/src/approval-handler.runtime.ts @@ -20,6 +20,7 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; import { logError } from "openclaw/plugin-sdk/logging-core"; import { normalizeOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime"; import { truncateUtf16Safe } from "openclaw/plugin-sdk/text-utility-runtime"; +import { SLACK_APPROVAL_HEADER_BLOCK_ID } from "./approval-actions.js"; import { isSlackAnyNativeApprovalClientEnabled, shouldHandleSlackNativeApprovalRequest, @@ -228,6 +229,7 @@ function buildSlackExecPendingApprovalBlocks(view: ExecApprovalPendingView): Sla return [ { type: "section", + block_id: SLACK_APPROVAL_HEADER_BLOCK_ID, text: { type: "mrkdwn", text: "*Exec approval required*\nA command needs your approval.", @@ -254,6 +256,7 @@ function buildSlackPluginPendingApprovalBlocks(view: PluginApprovalPendingView): return [ { type: "section", + block_id: SLACK_APPROVAL_HEADER_BLOCK_ID, text: { type: "mrkdwn", text: `*Plugin approval required*\n${truncateSlackMrkdwn( diff --git a/extensions/slack/src/monitor/events/interactions.block-actions.ts b/extensions/slack/src/monitor/events/interactions.block-actions.ts index 12541773f030..ac59427c276c 100644 --- a/extensions/slack/src/monitor/events/interactions.block-actions.ts +++ b/extensions/slack/src/monitor/events/interactions.block-actions.ts @@ -16,7 +16,11 @@ import { normalizeUniqueTrimmedStringList, } from "openclaw/plugin-sdk/string-coerce-runtime"; import { enqueueRoutedSystemEvent } from "openclaw/plugin-sdk/system-event-runtime"; -import { decodeSlackApprovalAction, type SlackApprovalAction } from "../../approval-actions.js"; +import { + decodeSlackApprovalAction, + SLACK_APPROVAL_HEADER_BLOCK_ID, + type SlackApprovalAction, +} from "../../approval-actions.js"; import { isSlackApprovalAuthorizedSender } from "../../approval-auth.js"; import { isSlackExecApprovalAuthorizedSender } from "../../exec-approvals.js"; import { dispatchSlackPluginInteractiveHandler } from "../../interactive-dispatch.js"; @@ -537,11 +541,9 @@ function buildSlackApprovalTerminalBlocks(params: { prefix: "Resolved" | "Already resolved"; }): (Block | KnownBlock)[] { const blocks = removeSlackApprovalControls(params.blocks ?? []).filter((block) => { - const text = (block as { type?: unknown; text?: { text?: unknown } }).text?.text; + const blockId = (block as { block_id?: unknown }).block_id; return !( - (block as { type?: unknown }).type === "section" && - typeof text === "string" && - /^\*(?:Exec|Plugin) approval required\*/u.test(text) + (block as { type?: unknown }).type === "section" && blockId === SLACK_APPROVAL_HEADER_BLOCK_ID ); }); return [ diff --git a/extensions/slack/src/monitor/events/interactions.test.ts b/extensions/slack/src/monitor/events/interactions.test.ts index 35006ee000b2..592308584fdd 100644 --- a/extensions/slack/src/monitor/events/interactions.test.ts +++ b/extensions/slack/src/monitor/events/interactions.test.ts @@ -1665,7 +1665,8 @@ describe("registerSlackInteractionEvents", () => { blocks: [ { type: "section", - text: { type: "mrkdwn", text: "*Exec approval required*\nA command needs approval." }, + block_id: "openclaw_approval_header", + text: { type: "mrkdwn", text: "Approval copy can change independently." }, }, { type: "section", text: { type: "mrkdwn", text: "Command preview" } }, {