diff --git a/extensions/signal/src/approval-handler.runtime.ts b/extensions/signal/src/approval-handler.runtime.ts index b6d1258bf0d5..485f6cc8be44 100644 --- a/extensions/signal/src/approval-handler.runtime.ts +++ b/extensions/signal/src/approval-handler.runtime.ts @@ -197,6 +197,7 @@ export const signalApprovalNativeRuntime = createChannelApprovalNativeRuntimeAda conversationKey: entry.conversationKey, messageId: entry.messageId, approvalId: request.id, + approvalKind: view.approvalKind, allowedDecisions: pendingPayload.reactionPayload.allowedDecisions, targetAuthorKeys: entry.targetAuthorKeys, route: { diff --git a/extensions/signal/src/approval-reactions.test.ts b/extensions/signal/src/approval-reactions.test.ts index 7ef35df75aa2..c6e084de3933 100644 --- a/extensions/signal/src/approval-reactions.test.ts +++ b/extensions/signal/src/approval-reactions.test.ts @@ -1,12 +1,16 @@ +import { + buildExecApprovalPendingReplyPayload, + buildPluginApprovalPendingReplyPayload, +} from "openclaw/plugin-sdk/approval-reply-runtime"; // Signal tests cover approval reactions plugin behavior. import { beforeEach, describe, expect, it, vi } from "vitest"; import { addSignalApprovalReactionHintToText, - appendSignalApprovalReactionHintForOutboundMessage, + addSignalApprovalReactionHintToStructuredPayload, buildSignalApprovalReactionHint, clearSignalApprovalReactionTargetsForTest, maybeResolveSignalApprovalReaction, - registerSignalApprovalReactionTargetForOutboundMessage, + registerSignalApprovalReactionTargetForDeliveredPayload, registerSignalApprovalReactionTarget, resolveSignalApprovalReactionTargetWithPersistence, } from "./approval-reactions.js"; @@ -78,7 +82,220 @@ describe("Signal approval reactions", () => { ).toBe(prompt); }); - it("registers target-mode outbound approval prompts for reactions", async () => { + it("registers delivered structured approval payloads for reactions", async () => { + const cfg = { + channels: { + signal: { + allowFrom: ["+15551230000"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets" as const, + targets: [{ channel: "signal", to: "+15551230000" }], + }, + }, + }; + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-structured-approval", + approvalSlug: "exec-str", + allowedDecisions: ["allow-once", "deny"], + command: "printf test", + host: "gateway", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }); + const deliveredPayload = addSignalApprovalReactionHintToStructuredPayload({ + cfg, + accountId: "default", + to: "+15551230000", + payload, + targetAuthor: "+15550009999", + }); + + expect( + registerSignalApprovalReactionTargetForDeliveredPayload({ + cfg, + target: { + channel: "signal", + to: "+15551230000", + accountId: "default", + }, + payload: deliveredPayload!, + results: [ + { + channel: "signal", + messageId: "1700000000012", + toJid: "+15551230000", + }, + ], + targetAuthor: "+15550009999", + }), + ).toBe(true); + + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: "+15551230000", + messageId: "1700000000012", + reactionKey: "šŸ‘", + targetAuthor: "+15550009999", + }), + ).resolves.toEqual({ + approvalId: "exec-structured-approval", + approvalKind: "exec", + decision: "allow-once", + route: { + deliveryMode: "target", + to: "+15551230000", + accountId: "default", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }, + }); + }); + + it("does not register metadata-only approval payloads without visible reaction hints", async () => { + const cfg = { + channels: { + signal: { + allowFrom: ["+15551230000"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets" as const, + targets: [{ channel: "signal", to: "+15551230000" }], + }, + }, + }; + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-hidden-reaction", + approvalSlug: "exec-hid", + allowedDecisions: ["allow-once", "deny"], + command: "printf hidden", + host: "gateway", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }); + + expect( + registerSignalApprovalReactionTargetForDeliveredPayload({ + cfg, + target: { + channel: "signal", + to: "+15551230000", + accountId: "default", + }, + payload, + results: [ + { + channel: "signal", + messageId: "1700000000015", + }, + ], + targetAuthor: "+15550009999", + }), + ).toBe(false); + + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: "+15551230000", + messageId: "1700000000015", + reactionKey: "šŸ‘", + targetAuthor: "+15550009999", + }), + ).resolves.toBeNull(); + }); + + it("registers only delivered chunks that contain visible reaction hints", async () => { + const cfg = { + channels: { + signal: { + allowFrom: ["+15551230000"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets" as const, + targets: [{ channel: "signal", to: "+15551230000" }], + }, + }, + }; + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-chunked-reaction", + approvalSlug: "exec-ch", + allowedDecisions: ["allow-once", "deny"], + command: "printf chunked", + host: "gateway", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }); + const deliveredPayload = addSignalApprovalReactionHintToStructuredPayload({ + cfg, + accountId: "default", + to: "+15551230000", + payload, + targetAuthor: "+15550009999", + }); + + expect( + registerSignalApprovalReactionTargetForDeliveredPayload({ + cfg, + target: { + channel: "signal", + to: "+15551230000", + accountId: "default", + }, + payload: deliveredPayload!, + results: [ + { + channel: "signal", + messageId: "1700000000016", + meta: { + signalVisibleText: "Exec approval required\n\nReact with:\n\nšŸ‘ Allow Once\nšŸ‘Ž Deny", + }, + }, + { + channel: "signal", + messageId: "1700000000017", + meta: { + signalVisibleText: "Continuation chunk without controls", + }, + }, + ], + targetAuthor: "+15550009999", + }), + ).toBe(true); + + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: "+15551230000", + messageId: "1700000000016", + reactionKey: "šŸ‘", + targetAuthor: "+15550009999", + }), + ).resolves.toMatchObject({ + approvalId: "exec-chunked-reaction", + decision: "allow-once", + }); + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: "+15551230000", + messageId: "1700000000017", + reactionKey: "šŸ‘", + targetAuthor: "+15550009999", + }), + ).resolves.toBeNull(); + }); + + it("registers delivered structured plugin approval payloads using metadata kind", async () => { const cfg = { channels: { signal: { @@ -93,70 +310,106 @@ describe("Signal approval reactions", () => { }, }, }; - const text = - "Plugin approval required\nID: plugin:abc\n\nReply with: /approve plugin:abc allow-once|deny"; - const textWithHint = appendSignalApprovalReactionHintForOutboundMessage({ + const payload = buildPluginApprovalPendingReplyPayload({ + request: { + id: "plugin-structured-approval", + request: { + title: "Sensitive plugin action", + description: "Needs approval", + allowedDecisions: ["allow-once", "deny"], + }, + createdAtMs: 1_000, + expiresAtMs: 61_000, + }, + nowMs: 1_000, + }); + const deliveredPayload = addSignalApprovalReactionHintToStructuredPayload({ cfg, accountId: "default", to: "+15551230000", - text, + payload, targetAuthor: "+15550009999", }); - expect(textWithHint).toContain("React with:\n\nšŸ‘ Allow Once\nšŸ‘Ž Deny"); expect( - registerSignalApprovalReactionTargetForOutboundMessage({ + registerSignalApprovalReactionTargetForDeliveredPayload({ cfg, - accountId: "default", - to: "+15551230000", - messageId: "1700000000009", - text: textWithHint, + target: { + channel: "signal", + to: "+15551230000", + accountId: "default", + }, + payload: deliveredPayload!, + results: [ + { + channel: "signal", + messageId: "1700000000013", + }, + ], targetAuthor: "+15550009999", }), ).toBe(true); - const handled = await maybeResolveSignalApprovalReaction({ - cfg, - accountId: "default", - conversationKey: "+15551230000", - messageId: "1700000000009", - reactionKey: "šŸ‘", - actorId: "+15551230000", - targetAuthor: "+15550009999", - }); - - expect(handled).toBe(true); - expect(resolverMocks.resolveSignalApproval).toHaveBeenCalledWith({ - cfg, - approvalId: "plugin:abc", + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: "+15551230000", + messageId: "1700000000013", + reactionKey: "šŸ‘", + targetAuthor: "+15550009999", + }), + ).resolves.toMatchObject({ + approvalId: "plugin-structured-approval", + approvalKind: "plugin", decision: "allow-once", - senderId: "+15551230000", - gatewayUrl: undefined, }); }); - it("keeps target-mode outbound prompts manual when the target route is disabled", () => { - const text = - "Plugin approval required\nID: plugin:abc\n\nReply with: /approve plugin:abc allow-once|deny"; + it("does not register delivered structured approval payloads without explicit approvers", () => { + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-no-approvers", + approvalSlug: "exec-no", + allowedDecisions: ["allow-once", "deny"], + command: "printf test", + host: "gateway", + }); + const deliveredPayload = { + ...payload, + text: addSignalApprovalReactionHintToText({ + text: payload.text ?? "", + allowedDecisions: ["allow-once", "deny"], + }), + }; expect( - appendSignalApprovalReactionHintForOutboundMessage({ + registerSignalApprovalReactionTargetForDeliveredPayload({ cfg: { - channels: { signal: { allowFrom: ["+15551230000"] } }, + channels: { + signal: {}, + }, approvals: { - plugin: { - enabled: false, + exec: { + enabled: true, mode: "targets", targets: [{ channel: "signal", to: "+15551230000" }], }, }, }, - accountId: "default", - to: "+15551230000", - text, + target: { + channel: "signal", + to: "+15551230000", + accountId: "default", + }, + payload: deliveredPayload, + results: [ + { + channel: "signal", + messageId: "1700000000014", + }, + ], targetAuthor: "+15550009999", }), - ).toBe(text); + ).toBe(false); }); it("registers reaction state when only allow-always is available", async () => { diff --git a/extensions/signal/src/approval-reactions.ts b/extensions/signal/src/approval-reactions.ts index 0b8f8731ae76..92a6bc423ddc 100644 --- a/extensions/signal/src/approval-reactions.ts +++ b/extensions/signal/src/approval-reactions.ts @@ -8,8 +8,12 @@ import { type ApprovalReactionDecisionBinding, type ApprovalReactionTargetRecord, } from "openclaw/plugin-sdk/approval-reaction-runtime"; -import type { ExecApprovalReplyDecision } from "openclaw/plugin-sdk/approval-reply-runtime"; +import { + getExecApprovalReplyMetadata, + type ExecApprovalReplyDecision, +} from "openclaw/plugin-sdk/approval-reply-runtime"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; +import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; import { normalizeAccountId } from "openclaw/plugin-sdk/routing"; import { normalizeLowercaseStringOrEmpty, @@ -21,7 +25,7 @@ import { looksLikeUuid } from "./identity.js"; import { normalizeSignalMessagingTarget } from "./normalize.js"; import { getOptionalSignalRuntime } from "./runtime.js"; -const PERSISTENT_NAMESPACE = "signal.approval-reactions"; +const PERSISTENT_NAMESPACE = "signal.approval-reactions.v2"; const PERSISTENT_MAX_ENTRIES = 1000; const DEFAULT_REACTION_TARGET_TTL_MS = 24 * 60 * 60 * 1000; @@ -58,6 +62,19 @@ type SignalApprovalReactionTarget = ApprovalReactionTargetRecord; +}; + let resolverRuntimePromise: Promise | undefined; const signalApprovalReactionTargets = @@ -320,7 +337,7 @@ export function addSignalApprovalReactionHintToText(params: { text: string; allowedDecisions: readonly ExecApprovalReplyDecision[]; }): string { - if (/(^|\n)React with:\s*(\n|$)/i.test(params.text)) { + if (hasSignalApprovalReactionHintText(params.text)) { return params.text; } const hint = buildSignalApprovalReactionHint(params.allowedDecisions); @@ -329,40 +346,8 @@ export function addSignalApprovalReactionHintToText(params: { : params.text; } -function normalizeApprovalDecision(value: string): ExecApprovalReplyDecision | null { - const normalized = value.trim().toLowerCase(); - if (normalized === "always") { - return "allow-always"; - } - if (normalized === "allow-once" || normalized === "allow-always" || normalized === "deny") { - return normalized; - } - return null; -} - -export function extractSignalApprovalPromptBinding(text: string): { - approvalId: string; - allowedDecisions: ExecApprovalReplyDecision[]; -} | null { - const allowedDecisions: ExecApprovalReplyDecision[] = []; - let approvalId = ""; - for (const line of text.split(/\r?\n/)) { - const match = line.match(/\/approve(?:@[^\s]+)?\s+([A-Za-z0-9][A-Za-z0-9._:-]*)\s+(.+)$/i); - if (!match) { - continue; - } - if (approvalId && match[1] !== approvalId) { - continue; - } - approvalId ||= match[1]; - for (const decisionText of match[2].split(/[\s|,]+/)) { - const decision = normalizeApprovalDecision(decisionText); - if (decision && !allowedDecisions.includes(decision)) { - allowedDecisions.push(decision); - } - } - } - return approvalId && allowedDecisions.length > 0 ? { approvalId, allowedDecisions } : null; +function hasSignalApprovalReactionHintText(text?: string | null): boolean { + return /(^|\n)React with:\s*(\n|$)/i.test(text ?? ""); } function buildTargetRoute(params: { @@ -370,6 +355,7 @@ function buildTargetRoute(params: { accountId?: string | null; to: string; approvalId: string; + approvalKind?: ApprovalKind; agentId?: string | null; sessionKey?: string | null; }): Extract | null { @@ -393,7 +379,7 @@ function buildTargetRoute(params: { return isSignalApprovalReactionRouteStillEnabled({ cfg: params.cfg, target: { - approvalKind: resolveApprovalKindFromId(params.approvalId), + approvalKind: params.approvalKind ?? resolveApprovalKindFromId(params.approvalId), route, }, }) @@ -401,64 +387,6 @@ function buildTargetRoute(params: { : null; } -export function shouldAppendSignalApprovalReactionHintForOutboundMessage(params: { - cfg: OpenClawConfig; - accountId?: string | null; - to: string; - text: string; - targetAuthor?: string | null; - targetAuthorUuid?: string | null; - agentId?: string | null; - sessionKey?: string | null; -}): boolean { - const binding = extractSignalApprovalPromptBinding(params.text); - if (!binding) { - return false; - } - if (resolveSignalApprovalTargetAuthorKeys(params).length === 0) { - return false; - } - if (!hasSignalApprovalReactionApprovers({ cfg: params.cfg, accountId: params.accountId })) { - return false; - } - return Boolean( - buildTargetRoute({ - cfg: params.cfg, - accountId: params.accountId, - to: params.to, - approvalId: binding.approvalId, - agentId: params.agentId, - sessionKey: params.sessionKey, - }), - ); -} - -export function appendSignalApprovalReactionHintForOutboundMessage(params: { - cfg: OpenClawConfig; - accountId?: string | null; - to: string; - text: string; - targetAuthor?: string | null; - targetAuthorUuid?: string | null; - agentId?: string | null; - sessionKey?: string | null; -}): string { - const binding = extractSignalApprovalPromptBinding(params.text); - if ( - !binding || - !shouldAppendSignalApprovalReactionHintForOutboundMessage({ - ...params, - text: params.text, - }) - ) { - return params.text; - } - return addSignalApprovalReactionHintToText({ - text: params.text, - allowedDecisions: binding.allowedDecisions, - }); -} - export function hasSignalApprovalReactionApprovers(params: { cfg: OpenClawConfig; accountId?: string | null; @@ -471,6 +399,7 @@ export function registerSignalApprovalReactionTarget(params: { conversationKey: string; messageId: string; approvalId: string; + approvalKind?: ApprovalKind; allowedDecisions: readonly ExecApprovalReplyDecision[]; targetAuthorKeys: readonly string[]; route: SignalApprovalReactionRoute; @@ -521,7 +450,7 @@ export function registerSignalApprovalReactionTarget(params: { } satisfies SignalApprovalReactionRoute); const target: SignalApprovalReactionTarget = { approvalId, - approvalKind: resolveApprovalKindFromId(approvalId), + approvalKind: params.approvalKind ?? resolveApprovalKindFromId(approvalId), allowedDecisions, targetAuthorKeys, route, @@ -530,50 +459,142 @@ export function registerSignalApprovalReactionTarget(params: { return target; } -export function registerSignalApprovalReactionTargetForOutboundMessage(params: { +export function addSignalApprovalReactionHintToStructuredPayload(params: { cfg: OpenClawConfig; - accountId: string; + accountId?: string | null; to: string; - messageId: string; - text: string; + payload: ReplyPayload; targetAuthor?: string | null; targetAuthorUuid?: string | null; - agentId?: string | null; - sessionKey?: string | null; - ttlMs?: number; -}): boolean { - const binding = extractSignalApprovalPromptBinding(params.text); - if (!binding) { - return false; +}): ReplyPayload | null { + const metadata = getExecApprovalReplyMetadata(params.payload); + if (!metadata?.allowedDecisions || metadata.allowedDecisions.length === 0) { + return null; } - const conversationKey = resolveSignalApprovalConversationKey(params.to); - if (!conversationKey) { - return false; + if (resolveSignalApprovalTargetAuthorKeys(params).length === 0) { + return null; + } + if (!hasSignalApprovalReactionApprovers({ cfg: params.cfg, accountId: params.accountId })) { + return null; } const route = buildTargetRoute({ cfg: params.cfg, accountId: params.accountId, to: params.to, - approvalId: binding.approvalId, - agentId: params.agentId, - sessionKey: params.sessionKey, + approvalId: metadata.approvalId, + approvalKind: metadata.approvalKind, + agentId: metadata.agentId, + sessionKey: metadata.sessionKey, + }); + if (!route || !params.payload.text) { + return null; + } + return { + ...params.payload, + text: addSignalApprovalReactionHintToText({ + text: params.payload.text, + allowedDecisions: metadata.allowedDecisions, + }), + }; +} + +function readSignalDeliveryVisibleText(result: SignalApprovalDeliveryResult): string | null { + const meta = result.meta; + const visibleText = meta?.signalVisibleText ?? meta?.visibleText; + return typeof visibleText === "string" ? visibleText : null; +} + +function listDeliveredSignalMessageIdsWithVisibleHint(params: { + payload: ReplyPayload; + results: readonly SignalApprovalDeliveryResult[]; +}): string[] { + const signalResults = params.results.filter( + (result) => !result.channel || normalizeLowercaseStringOrEmpty(result.channel) === "signal", + ); + const resultsWithVisibleText = signalResults.filter( + (result) => readSignalDeliveryVisibleText(result) !== null, + ); + const candidates = resultsWithVisibleText.length > 0 ? resultsWithVisibleText : signalResults; + if (resultsWithVisibleText.length === 0 && candidates.length !== 1) { + return []; + } + const ids = candidates + .filter((result) => + resultsWithVisibleText.length > 0 + ? hasSignalApprovalReactionHintText(readSignalDeliveryVisibleText(result)) + : hasSignalApprovalReactionHintText(params.payload.text), + ) + .map((result) => normalizeOptionalString(result.messageId)) + .filter((messageId): messageId is string => Boolean(messageId && messageId !== "unknown")); + return Array.from(new Set(ids)); +} + +export function registerSignalApprovalReactionTargetForDeliveredPayload(params: { + cfg: OpenClawConfig; + target: SignalApprovalDeliveryTarget; + payload: ReplyPayload; + results: readonly SignalApprovalDeliveryResult[]; + targetAuthor?: string | null; + targetAuthorUuid?: string | null; + ttlMs?: number; +}): boolean { + if (normalizeLowercaseStringOrEmpty(params.target.channel) !== "signal") { + return false; + } + const metadata = getExecApprovalReplyMetadata(params.payload); + if (!metadata?.allowedDecisions || metadata.allowedDecisions.length === 0) { + return false; + } + if (!hasSignalApprovalReactionHintText(params.payload.text)) { + return false; + } + if ( + !hasSignalApprovalReactionApprovers({ cfg: params.cfg, accountId: params.target.accountId }) + ) { + return false; + } + const conversationKey = resolveSignalApprovalConversationKey(params.target.to); + if (!conversationKey) { + return false; + } + const route = buildTargetRoute({ + cfg: params.cfg, + accountId: params.target.accountId, + to: params.target.to, + approvalId: metadata.approvalId, + approvalKind: metadata.approvalKind, + agentId: metadata.agentId, + sessionKey: metadata.sessionKey, }); if (!route) { return false; } - return Boolean( - registerSignalApprovalReactionTarget({ - accountId: params.accountId, - conversationKey, - messageId: params.messageId, - approvalId: binding.approvalId, - allowedDecisions: binding.allowedDecisions, - targetAuthorKeys: resolveSignalApprovalTargetAuthorKeys(params), - route, - routeAllowed: true, - ttlMs: params.ttlMs, - }), - ); + const targetAuthorKeys = resolveSignalApprovalTargetAuthorKeys(params); + if (targetAuthorKeys.length === 0) { + return false; + } + let registered = false; + for (const messageId of listDeliveredSignalMessageIdsWithVisibleHint({ + payload: params.payload, + results: params.results, + })) { + registered = + Boolean( + registerSignalApprovalReactionTarget({ + accountId: normalizeAccountId(params.target.accountId ?? undefined), + conversationKey, + messageId, + approvalId: metadata.approvalId, + approvalKind: metadata.approvalKind, + allowedDecisions: metadata.allowedDecisions, + targetAuthorKeys, + route, + routeAllowed: true, + ttlMs: params.ttlMs, + }), + ) || registered; + } + return registered; } export function unregisterSignalApprovalReactionTarget(params: { diff --git a/extensions/signal/src/channel.ts b/extensions/signal/src/channel.ts index 8ad5a639fb6c..a271935395ff 100644 --- a/extensions/signal/src/channel.ts +++ b/extensions/signal/src/channel.ts @@ -1,6 +1,7 @@ // Signal plugin module implements channel behavior. import { DEFAULT_ACCOUNT_ID } from "openclaw/plugin-sdk/account-id"; import { buildDmGroupAccountAllowlistAdapter } from "openclaw/plugin-sdk/allowlist-config-edit"; +import type { ChannelOutboundAdapter } from "openclaw/plugin-sdk/channel-contract"; import { createChatChannelPlugin, type ChannelPlugin } from "openclaw/plugin-sdk/channel-core"; import { defineChannelMessageAdapter } from "openclaw/plugin-sdk/channel-outbound"; import { resolveOutboundSendDep } from "openclaw/plugin-sdk/channel-outbound"; @@ -40,10 +41,12 @@ import { } from "./shared.js"; type SignalSendFn = typeof import("./send.runtime.js").sendMessageSignal; type SignalProbe = import("./probe.js").SignalProbe; +type SignalApprovalReactionsModule = typeof import("./approval-reactions.js"); let signalMonitorModulePromise: Promise | null = null; let signalProbeModulePromise: Promise | null = null; let signalSendRuntimePromise: Promise | null = null; +let signalApprovalReactionsModulePromise: Promise | null = null; async function loadSignalMonitorModule() { signalMonitorModulePromise ??= import("./monitor.js"); @@ -60,6 +63,11 @@ async function loadSignalSendRuntime() { return await signalSendRuntimePromise; } +async function loadSignalApprovalReactionsModule() { + signalApprovalReactionsModulePromise ??= import("./approval-reactions.js"); + return await signalApprovalReactionsModulePromise; +} + async function resolveSignalSendContext(params: { cfg: Parameters[0]["cfg"]; accountId?: string; @@ -102,6 +110,20 @@ type SignalMessageContextExtras = { deps?: { [channelId: string]: unknown }; }; +function attachSignalVisibleText(result: T, visibleText: string) { + const meta = + "meta" in result && result.meta && typeof result.meta === "object" + ? (result.meta as Record) + : {}; + return { + ...result, + meta: { + ...meta, + signalVisibleText: visibleText, + }, + }; +} + const signalMessageAdapter = defineChannelMessageAdapter({ id: "signal", durableFinal: { @@ -224,7 +246,7 @@ async function sendFormattedSignalText(ctx: { textMode: "plain", textStyles: chunk.styles, }); - results.push(result); + results.push(attachSignalVisibleText(result, chunk.text)); } return attachChannelToResults("signal", results); } @@ -267,7 +289,49 @@ async function sendFormattedSignalMedia(ctx: { textMode: "plain", textStyles: formatted.styles, }); - return attachChannelToResult("signal", result); + return attachChannelToResult("signal", attachSignalVisibleText(result, formatted.text)); +} + +async function registerDeliveredSignalApprovalPayloadForReactions( + params: Parameters>[0], +) { + const account = resolveSignalAccount({ + cfg: params.cfg, + accountId: params.target.accountId ?? undefined, + }); + if (!account.config.account) { + return; + } + const { registerSignalApprovalReactionTargetForDeliveredPayload } = + await loadSignalApprovalReactionsModule(); + registerSignalApprovalReactionTargetForDeliveredPayload({ + cfg: params.cfg, + target: params.target, + payload: params.payload, + results: params.results, + targetAuthor: account.config.account, + }); +} + +async function renderSignalApprovalPayloadForReactions( + params: Parameters>[0], +) { + const account = resolveSignalAccount({ + cfg: params.ctx.cfg, + accountId: params.ctx.accountId ?? undefined, + }); + if (!account.config.account) { + return null; + } + const { addSignalApprovalReactionHintToStructuredPayload } = + await loadSignalApprovalReactionsModule(); + return addSignalApprovalReactionHintToStructuredPayload({ + cfg: params.ctx.cfg, + accountId: params.ctx.accountId ?? undefined, + to: params.ctx.to, + payload: params.payload, + targetAuthor: account.config.account, + }); } export const signalPlugin: ChannelPlugin = @@ -404,6 +468,9 @@ export const signalPlugin: ChannelPlugin = payload, hint, }), + afterDeliverPayload: async (params) => + await registerDeliveredSignalApprovalPayloadForReactions(params), + renderPresentation: async (params) => await renderSignalApprovalPayloadForReactions(params), sendFormattedText: async ({ cfg, to, text, accountId, deps, abortSignal }) => await sendFormattedSignalText({ cfg, diff --git a/extensions/signal/src/core.test.ts b/extensions/signal/src/core.test.ts index aa4225486bba..fe34a6b28709 100644 --- a/extensions/signal/src/core.test.ts +++ b/extensions/signal/src/core.test.ts @@ -1,3 +1,4 @@ +import { buildExecApprovalPendingReplyPayload } from "openclaw/plugin-sdk/approval-reply-runtime"; // Signal tests cover core plugin behavior. import { createMessageReceiptFromOutboundResults, @@ -6,6 +7,10 @@ import { import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; import { createPluginSetupWizardStatus } from "openclaw/plugin-sdk/plugin-test-runtime"; import { describe, expect, it, vi } from "vitest"; +import { + clearSignalApprovalReactionTargetsForTest, + resolveSignalApprovalReactionTargetWithPersistence, +} from "./approval-reactions.js"; import { signalPlugin } from "./channel.js"; import * as clientModule from "./client-adapter.js"; import { classifySignalCliLogLine } from "./daemon.js"; @@ -264,6 +269,143 @@ describe("signal outbound", () => { ).toBe(true); }); + it("registers structured approval payloads for reactions after delivery", async () => { + clearSignalApprovalReactionTargetsForTest(); + const cfg = { + channels: { + signal: { + account: "+15550009999", + allowFrom: ["+15551230000"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets", + targets: [{ channel: "signal", to: "+15551230000" }], + }, + }, + } as OpenClawConfig; + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-after-delivery", + approvalSlug: "exec-aft", + allowedDecisions: ["allow-once", "deny"], + command: "printf test", + host: "gateway", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }); + const rendered = await signalPlugin.outbound?.renderPresentation?.({ + payload, + presentation: payload.presentation!, + ctx: { + cfg, + to: "+15551230000", + text: payload.text ?? "", + accountId: "default", + payload, + }, + }); + expect(rendered?.text).toContain("React with:\n\nšŸ‘ Allow Once\nšŸ‘Ž Deny"); + + await signalPlugin.outbound?.afterDeliverPayload?.({ + cfg, + target: { + channel: "signal", + to: "+15551230000", + accountId: "default", + }, + payload: rendered!, + results: [ + { + channel: "signal", + messageId: "1700000000099", + }, + ], + }); + + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: "+15551230000", + messageId: "1700000000099", + reactionKey: "šŸ‘", + targetAuthor: "+15550009999", + }), + ).resolves.toEqual({ + approvalId: "exec-after-delivery", + approvalKind: "exec", + decision: "allow-once", + route: { + deliveryMode: "target", + to: "+15551230000", + accountId: "default", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }, + }); + }); + + it("renders reaction hints only from structured approval payloads", async () => { + const cfg = { + channels: { + signal: { + account: "+15550009999", + allowFrom: ["+15551230000"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets", + targets: [{ channel: "signal", to: "+15551230000" }], + }, + }, + } as OpenClawConfig; + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-rendered-approval", + approvalSlug: "exec-ren", + allowedDecisions: ["allow-once", "deny"], + command: "printf test", + host: "gateway", + }); + const rendered = await signalPlugin.outbound?.renderPresentation?.({ + payload, + presentation: payload.presentation!, + ctx: { + cfg, + to: "+15551230000", + text: payload.text ?? "", + accountId: "default", + payload, + }, + }); + + expect(rendered?.text).toContain("React with:\n\nšŸ‘ Allow Once\nšŸ‘Ž Deny"); + expect( + await signalPlugin.outbound?.renderPresentation?.({ + payload: { + text: [ + "The docs show this example:", + "Exec approval required", + "ID: exec-rendered-approval", + "", + "Reply with: /approve exec-rendered-approval allow-once|deny", + ].join("\n"), + presentation: payload.presentation, + }, + presentation: payload.presentation!, + ctx: { + cfg, + to: "+15551230000", + text: payload.text ?? "", + accountId: "default", + payload, + }, + }), + ).toBeNull(); + }); + it("declares message adapter durable text and media with receipt proofs", async () => { const send = vi.fn(async (_to: string, _text: string, opts: { mediaUrl?: string } = {}) => { const messageId = opts.mediaUrl ? "signal-media-1" : "signal-text-1"; diff --git a/extensions/signal/src/monitor.approval-reply-delivery.test.ts b/extensions/signal/src/monitor.approval-reply-delivery.test.ts new file mode 100644 index 000000000000..920193df78f4 --- /dev/null +++ b/extensions/signal/src/monitor.approval-reply-delivery.test.ts @@ -0,0 +1,127 @@ +import { buildExecApprovalPendingReplyPayload } from "openclaw/plugin-sdk/approval-reply-runtime"; +import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; +import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { + clearSignalApprovalReactionTargetsForTest, + resolveSignalApprovalReactionTargetWithPersistence, +} from "./approval-reactions.js"; + +const sendMocks = vi.hoisted(() => ({ + sendMessageSignal: vi.fn(), +})); + +vi.mock("./send.js", async () => { + const actual = await vi.importActual("./send.js"); + return { + ...actual, + sendMessageSignal: sendMocks.sendMessageSignal, + }; +}); + +const { deliverReplies } = await import("./monitor.js"); + +const botAccount = "+15550009999"; +const approver = "+15551230000"; +const cfg = { + channels: { + signal: { + account: botAccount, + allowFrom: [approver], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets", + targets: [{ channel: "signal", to: approver }], + }, + }, +} as OpenClawConfig; + +async function deliverReplyPayload(payload: ReplyPayload) { + await deliverReplies({ + cfg, + replies: [payload], + target: approver, + baseUrl: "http://127.0.0.1:8080", + account: botAccount, + accountId: "default", + runtime: { log: vi.fn() } as never, + maxBytes: 8 * 1024 * 1024, + textLimit: 4000, + chunkMode: "length", + }); +} + +describe("Signal monitor approval reply delivery", () => { + beforeEach(() => { + clearSignalApprovalReactionTargetsForTest(); + sendMocks.sendMessageSignal.mockReset().mockResolvedValue({ + messageId: "1700000000200", + }); + }); + + it("adds reaction hints and registers structured approval replies delivered by the monitor", async () => { + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "exec-monitor-structured", + approvalSlug: "exec-mon", + allowedDecisions: ["allow-once", "deny"], + command: "printf monitor", + host: "gateway", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }); + + await deliverReplyPayload(payload); + + const sentText = String(sendMocks.sendMessageSignal.mock.calls[0]?.[1] ?? ""); + expect(sentText).toContain("React with:\n\nšŸ‘ Allow Once\nšŸ‘Ž Deny"); + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: approver, + messageId: "1700000000200", + reactionKey: "šŸ‘", + targetAuthor: botAccount, + }), + ).resolves.toEqual({ + approvalId: "exec-monitor-structured", + approvalKind: "exec", + decision: "allow-once", + route: { + deliveryMode: "target", + to: approver, + accountId: "default", + agentId: "main", + sessionKey: "agent:main:signal:direct:+15551230000", + }, + }); + }); + + it("does not bind ordinary monitor replies that quote approval commands", async () => { + const payload = { + text: [ + "The docs show this example:", + "Exec approval required", + "ID: exec-monitor-quoted", + "", + "Reply with: /approve exec-monitor-quoted allow-once|deny", + ].join("\n"), + }; + + await deliverReplyPayload(payload); + + const sentText = String(sendMocks.sendMessageSignal.mock.calls[0]?.[1] ?? ""); + expect(sentText).not.toContain("React with:"); + await expect( + resolveSignalApprovalReactionTargetWithPersistence({ + accountId: "default", + conversationKey: approver, + messageId: "1700000000200", + reactionKey: "šŸ‘", + targetAuthor: botAccount, + }), + ).resolves.toBeNull(); + }); +}); diff --git a/extensions/signal/src/monitor.ts b/extensions/signal/src/monitor.ts index bf5cc395a714..5f2c01580de7 100644 --- a/extensions/signal/src/monitor.ts +++ b/extensions/signal/src/monitor.ts @@ -39,6 +39,10 @@ import { normalizeE164 } from "openclaw/plugin-sdk/text-utility-runtime"; import { waitForTransportReady } from "openclaw/plugin-sdk/transport-ready-runtime"; import { resolveSignalAccount } from "./accounts.js"; import { isSignalNativeApprovalHandlerConfigured } from "./approval-native.js"; +import { + addSignalApprovalReactionHintToStructuredPayload, + registerSignalApprovalReactionTargetForDeliveredPayload, +} from "./approval-reactions.js"; import { signalRpcRequest, signalCheck } from "./client-adapter.js"; import { formatSignalDaemonExit, spawnSignalDaemon, type SignalDaemonHandle } from "./daemon.js"; import { isSignalSenderAllowed, type resolveSignalSender } from "./identity.js"; @@ -354,7 +358,7 @@ async function fetchAttachment(params: { return { path: saved.path, contentType: saved.contentType }; } -async function deliverReplies(params: { +export async function deliverReplies(params: { cfg: OpenClawConfig; replies: ReplyPayload[]; target: string; @@ -369,32 +373,79 @@ async function deliverReplies(params: { const { replies, target, baseUrl, account, accountId, runtime, maxBytes, textLimit, chunkMode } = params; for (const payload of replies) { - const reply = resolveSendableOutboundReplyParts(payload); + const deliveryResults: Array<{ + channel: "signal"; + messageId: string; + meta: { signalVisibleText: string }; + }> = []; + const deliveredPayload = + addSignalApprovalReactionHintToStructuredPayload({ + cfg: params.cfg, + accountId, + to: target, + payload, + targetAuthor: account, + }) ?? payload; + const reply = resolveSendableOutboundReplyParts(deliveredPayload); + const recordDeliveryResult = ( + result: Awaited>, + visibleText: string, + ) => { + const messageId = + typeof result?.messageId === "string" && result.messageId.trim() + ? result.messageId.trim() + : null; + if (messageId) { + deliveryResults.push({ + channel: "signal", + messageId, + meta: { signalVisibleText: visibleText }, + }); + } + }; const delivered = await deliverTextOrMediaReply({ - payload, + payload: deliveredPayload, text: reply.text, chunkText: (value) => chunkTextWithMode(value, textLimit, chunkMode), sendText: async (chunk) => { - await sendMessageSignal(target, chunk, { - cfg: params.cfg, - baseUrl, - account, - maxBytes, - accountId, - }); + recordDeliveryResult( + await sendMessageSignal(target, chunk, { + cfg: params.cfg, + baseUrl, + account, + maxBytes, + accountId, + }), + chunk, + ); }, sendMedia: async ({ mediaUrl, caption }) => { - await sendMessageSignal(target, caption ?? "", { - cfg: params.cfg, - baseUrl, - account, - mediaUrl, - maxBytes, - accountId, - }); + const visibleText = caption ?? ""; + recordDeliveryResult( + await sendMessageSignal(target, visibleText, { + cfg: params.cfg, + baseUrl, + account, + mediaUrl, + maxBytes, + accountId, + }), + visibleText, + ); }, }); if (delivered !== "empty") { + registerSignalApprovalReactionTargetForDeliveredPayload({ + cfg: params.cfg, + target: { + channel: "signal", + to: target, + accountId, + }, + payload: deliveredPayload, + results: deliveryResults, + targetAuthor: account, + }); runtime.log?.(`delivered reply to ${target}`); } } diff --git a/extensions/signal/src/send.test.ts b/extensions/signal/src/send.test.ts index 8481926cf2e8..51b043be9bee 100644 --- a/extensions/signal/src/send.test.ts +++ b/extensions/signal/src/send.test.ts @@ -129,4 +129,73 @@ describe("sendMessageSignal receipts", () => { expect(result.messageId).toBe("unknown"); expect(result.receipt.platformMessageIds).toStrictEqual([]); }); + + it("does not add approval reactions to ordinary outbound approval-looking text", async () => { + signalRpcRequestMock.mockResolvedValueOnce({ timestamp: 1234567892 }); + const text = [ + "Here is the command you asked about:", + "/approve exec-live-approval allow-once|deny", + ].join("\n"); + + await sendMessageSignal("+15551234567", text, { + cfg: { + ...SIGNAL_TEST_CFG, + channels: { + signal: { + ...SIGNAL_TEST_CFG.channels.signal, + allowFrom: ["+15551234567"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets", + targets: [{ channel: "signal", to: "+15551234567" }], + }, + }, + }, + }); + + expect(signalRpcRequestMock).toHaveBeenCalledWith( + "send", + expect.objectContaining({ message: text }), + expect.any(Object), + ); + }); + + it("does not add approval reactions to ordinary outbound text quoting a full prompt", async () => { + signalRpcRequestMock.mockResolvedValueOnce({ timestamp: 1234567893 }); + const text = [ + "The docs show this example:", + "Exec approval required", + "ID: exec-live-approval", + "", + "Reply with: /approve exec-live-approval allow-once|deny", + ].join("\n"); + + await sendMessageSignal("+15551234567", text, { + cfg: { + ...SIGNAL_TEST_CFG, + channels: { + signal: { + ...SIGNAL_TEST_CFG.channels.signal, + allowFrom: ["+15551234567"], + }, + }, + approvals: { + exec: { + enabled: true, + mode: "targets", + targets: [{ channel: "signal", to: "+15551234567" }], + }, + }, + }, + }); + + expect(signalRpcRequestMock).toHaveBeenCalledWith( + "send", + expect.objectContaining({ message: text }), + expect.any(Object), + ); + }); }); diff --git a/extensions/signal/src/send.ts b/extensions/signal/src/send.ts index 57157eb19e0d..c24c300ef159 100644 --- a/extensions/signal/src/send.ts +++ b/extensions/signal/src/send.ts @@ -12,10 +12,6 @@ import { resolveOutboundAttachmentFromUrl } from "openclaw/plugin-sdk/media-runt import { requireRuntimeConfig } from "openclaw/plugin-sdk/plugin-config-runtime"; import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/string-coerce-runtime"; import { resolveSignalAccount } from "./accounts.js"; -import { - appendSignalApprovalReactionHintForOutboundMessage, - registerSignalApprovalReactionTargetForOutboundMessage, -} from "./approval-reactions.js"; import { signalRpcRequest } from "./client-adapter.js"; import { markdownToSignalText, type SignalTextStyleRange } from "./format.js"; import { resolveSignalRpcContext } from "./rpc-context.js"; @@ -184,14 +180,7 @@ export async function sendMessageSignal( }); const { baseUrl, account } = resolveSignalRpcContext(opts, accountInfo); const target = parseTarget(to); - const outboundText = appendSignalApprovalReactionHintForOutboundMessage({ - cfg, - accountId: accountInfo.accountId, - to, - text: text ?? "", - targetAuthor: account, - }); - let message = outboundText; + let message = text ?? ""; let messageFromPlaceholder = false; let textStyles: SignalTextStyleRange[] = []; const textMode = opts.textMode ?? "markdown"; @@ -273,14 +262,6 @@ export async function sendMessageSignal( }); const timestamp = result?.timestamp; const messageId = timestamp ? String(timestamp) : "unknown"; - registerSignalApprovalReactionTargetForOutboundMessage({ - cfg, - accountId: accountInfo.accountId, - to, - messageId, - text: outboundText, - targetAuthor: account, - }); return { messageId, timestamp,