From 15c3aa82bffd64a2d1f3502df05e12af729bc723 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 30 Mar 2026 08:27:37 +0900 Subject: [PATCH] refactor: unify approval forwarding and rendering --- .../discord/src/outbound-adapter.test.ts | 46 ++ extensions/discord/src/outbound-adapter.ts | 32 +- src/infra/exec-approval-forwarder.test.ts | 9 +- src/infra/exec-approval-forwarder.ts | 670 ++++++++++-------- src/infra/plugin-approval-forwarder.test.ts | 12 +- src/plugin-sdk/approval-renderers.test.ts | 26 +- src/plugin-sdk/approval-renderers.ts | 49 +- 7 files changed, 518 insertions(+), 326 deletions(-) diff --git a/extensions/discord/src/outbound-adapter.test.ts b/extensions/discord/src/outbound-adapter.test.ts index dbc71f9b467..084e2a9a9f0 100644 --- a/extensions/discord/src/outbound-adapter.test.ts +++ b/extensions/discord/src/outbound-adapter.test.ts @@ -240,4 +240,50 @@ describe("discordOutbound", () => { channelId: "ch-1", }); }); + + it("neutralizes approval mentions only for approval payloads", async () => { + await discordOutbound.sendPayload?.({ + cfg: {}, + to: "channel:123456", + text: "", + payload: { + text: "Approval @everyone <@123> <#456>", + channelData: { + execApproval: { + approvalId: "req-1", + approvalSlug: "req-1", + }, + }, + }, + accountId: "default", + }); + + expect(hoisted.sendMessageDiscordMock).toHaveBeenCalledWith( + "channel:123456", + "Approval @\u200beveryone <@\u200b123> <#\u200b456>", + expect.objectContaining({ + accountId: "default", + }), + ); + }); + + it("leaves non-approval mentions unchanged", async () => { + await discordOutbound.sendPayload?.({ + cfg: {}, + to: "channel:123456", + text: "", + payload: { + text: "Hello @everyone", + }, + accountId: "default", + }); + + expect(hoisted.sendMessageDiscordMock).toHaveBeenCalledWith( + "channel:123456", + "Hello @everyone", + expect.objectContaining({ + accountId: "default", + }), + ); + }); }); diff --git a/extensions/discord/src/outbound-adapter.ts b/extensions/discord/src/outbound-adapter.ts index e0552eeb370..f79f6403635 100644 --- a/extensions/discord/src/outbound-adapter.ts +++ b/extensions/discord/src/outbound-adapter.ts @@ -26,6 +26,33 @@ import { buildDiscordInteractiveComponents } from "./shared-interactive.js"; export const DISCORD_TEXT_CHUNK_LIMIT = 2000; +function hasApprovalChannelData(payload: { channelData?: unknown }): boolean { + const channelData = payload.channelData; + if (!channelData || typeof channelData !== "object" || Array.isArray(channelData)) { + return false; + } + return Boolean((channelData as { execApproval?: unknown }).execApproval); +} + +function neutralizeDiscordApprovalMentions(value: string): string { + return value + .replace(/@everyone/gi, "@\u200beveryone") + .replace(/@here/gi, "@\u200bhere") + .replace(/<@/g, "<@\u200b") + .replace(/<#/g, "<#\u200b"); +} + +function normalizeDiscordApprovalPayload( + payload: T, +): T { + return hasApprovalChannelData(payload) && payload.text + ? { + ...payload, + text: neutralizeDiscordApprovalMentions(payload.text), + } + : payload; +} + function resolveDiscordOutboundTarget(params: { to: string; threadId?: string | number | null; @@ -96,12 +123,13 @@ export const discordOutbound: ChannelOutboundAdapter = { chunker: null, textChunkLimit: DISCORD_TEXT_CHUNK_LIMIT, pollMaxOptions: 10, + normalizePayload: ({ payload }) => normalizeDiscordApprovalPayload(payload), resolveTarget: ({ to }) => normalizeDiscordOutboundTarget(to), sendPayload: async (ctx) => { - const payload = { + const payload = normalizeDiscordApprovalPayload({ ...ctx.payload, text: ctx.payload.text ?? "", - }; + }); const discordData = payload.channelData?.discord as | { components?: DiscordComponentMessageSpec } | undefined; diff --git a/src/infra/exec-approval-forwarder.test.ts b/src/infra/exec-approval-forwarder.test.ts index 309dba1736a..b02b7c467f0 100644 --- a/src/infra/exec-approval-forwarder.test.ts +++ b/src/infra/exec-approval-forwarder.test.ts @@ -45,10 +45,10 @@ function isDiscordExecApprovalClientEnabledForTest(params: { const telegramApprovalPlugin: Pick< ChannelPlugin, - "id" | "meta" | "capabilities" | "config" | "execApprovals" + "id" | "meta" | "capabilities" | "config" | "approvals" > = { ...createChannelTestPluginBase({ id: "telegram" }), - execApprovals: { + approvals: { delivery: { shouldSuppressForwardingFallback: (params) => shouldSuppressTelegramExecApprovalForwardingFallback(params), @@ -57,10 +57,10 @@ const telegramApprovalPlugin: Pick< }; const discordApprovalPlugin: Pick< ChannelPlugin, - "id" | "meta" | "capabilities" | "config" | "execApprovals" + "id" | "meta" | "capabilities" | "config" | "approvals" > = { ...createChannelTestPluginBase({ id: "discord" }), - execApprovals: { + approvals: { delivery: { shouldSuppressForwardingFallback: ({ cfg, target }) => target.channel === "discord" && @@ -426,7 +426,6 @@ describe("exec approval forwarder", () => { }); it("can forward resolved notices without pending cache when request payload is present", async () => { - vi.useFakeTimers(); const { deliver, forwarder } = createForwarder({ cfg: makeTargetsCfg([{ channel: "telegram", to: "123" }]), }); diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index 335e47068bf..6a790dc22da 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/src/infra/exec-approval-forwarder.ts @@ -1,5 +1,5 @@ import type { ReplyPayload } from "../auto-reply/types.js"; -import { getChannelPlugin } from "../channels/plugins/index.js"; +import { getChannelPlugin, resolveChannelApprovalAdapter } from "../channels/plugins/index.js"; import type { OpenClawConfig } from "../config/config.js"; import { loadConfig } from "../config/config.js"; import type { @@ -9,7 +9,9 @@ import type { import { createSubsystemLogger } from "../logging/subsystem.js"; import { buildApprovalPendingReplyPayload, + buildApprovalResolvedReplyPayload, buildPluginApprovalPendingReplyPayload, + buildPluginApprovalResolvedReplyPayload, } from "../plugin-sdk/approval-renderers.js"; import { parseAgentSessionKey } from "../routing/session-key.js"; import { compileConfigRegex } from "../security/config-regex.js"; @@ -28,7 +30,6 @@ import { approvalDecisionLabel, buildPluginApprovalExpiredMessage, buildPluginApprovalRequestMessage, - buildPluginApprovalResolvedMessage, type PluginApprovalRequest, type PluginApprovalResolved, } from "./plugin-approvals.js"; @@ -36,14 +37,66 @@ import { const log = createSubsystemLogger("gateway/exec-approvals"); export type { ExecApprovalRequest, ExecApprovalResolved }; +type ApprovalKind = "exec" | "plugin"; type ForwardTarget = ExecApprovalForwardTarget & { source: "session" | "target" }; -type PendingApproval = { - request: ExecApprovalRequest; +type ApprovalRouteRequest = { + agentId?: string | null; + sessionKey?: string | null; + turnSourceChannel?: string | null; + turnSourceTo?: string | null; + turnSourceAccountId?: string | null; + turnSourceThreadId?: string | number | null; +}; + +type PendingApproval = { + routeRequest: TRouteRequest; targets: ForwardTarget[]; timeoutId: NodeJS.Timeout | null; }; +type ApprovalRenderContext = { + cfg: OpenClawConfig; + target: ForwardTarget; + routeRequest: TRouteRequest; +}; + +type ApprovalPendingRenderContext< + TRequest, + TRouteRequest extends ApprovalRouteRequest, +> = ApprovalRenderContext & { + request: TRequest; + nowMs: number; +}; + +type ApprovalResolvedRenderContext< + TResolved, + TRouteRequest extends ApprovalRouteRequest, +> = ApprovalRenderContext & { + resolved: TResolved; +}; + +type ApprovalStrategy< + TRequest, + TResolved, + TRouteRequest extends ApprovalRouteRequest = ApprovalRouteRequest, +> = { + kind: ApprovalKind; + config: (cfg: OpenClawConfig) => ExecApprovalForwardingConfig | undefined; + getRequestId: (request: TRequest) => string; + getResolvedId: (resolved: TResolved) => string; + getExpiresAtMs: (request: TRequest) => number; + getRouteRequestFromRequest: (request: TRequest) => TRouteRequest; + getRouteRequestFromResolved: (resolved: TResolved) => TRouteRequest | null; + buildExpiredText: (request: TRequest) => string; + buildPendingPayload: ( + params: ApprovalPendingRenderContext, + ) => ReplyPayload; + buildResolvedPayload: ( + params: ApprovalResolvedRenderContext, + ) => ReplyPayload; +}; + export type ExecApprovalForwarder = { handleRequested: (request: ExecApprovalRequest) => Promise; handleResolved: (resolved: ExecApprovalResolved) => Promise; @@ -63,6 +116,7 @@ export type ExecApprovalForwarderDeps = { }; const DEFAULT_MODE = "session" as const; +const SYNTHETIC_APPROVAL_REQUEST_ID = "__approval-routing__"; function normalizeMode(mode?: ExecApprovalForwardingConfig["mode"]) { return mode ?? DEFAULT_MODE; @@ -78,13 +132,13 @@ function matchSessionFilter(sessionKey: string, patterns: string[]): boolean { }); } -function shouldForward(params: { +function shouldForwardRoute(params: { config?: { enabled?: boolean; agentFilter?: string[]; sessionFilter?: string[]; }; - request: ExecApprovalRequest; + routeRequest: ApprovalRouteRequest; }): boolean { const config = params.config; if (!config?.enabled) { @@ -92,21 +146,14 @@ function shouldForward(params: { } if (config.agentFilter?.length) { const agentId = - params.request.request.agentId ?? - parseAgentSessionKey(params.request.request.sessionKey)?.agentId; - if (!agentId) { - return false; - } - if (!config.agentFilter.includes(agentId)) { + params.routeRequest.agentId ?? parseAgentSessionKey(params.routeRequest.sessionKey)?.agentId; + if (!agentId || !config.agentFilter.includes(agentId)) { return false; } } if (config.sessionFilter?.length) { - const sessionKey = params.request.request.sessionKey; - if (!sessionKey) { - return false; - } - if (!matchSessionFilter(sessionKey, config.sessionFilter)) { + const sessionKey = params.routeRequest.sessionKey; + if (!sessionKey || !matchSessionFilter(sessionKey, config.sessionFilter)) { return false; } } @@ -120,21 +167,38 @@ function buildTargetKey(target: ExecApprovalForwardTarget): string { return [channel, target.to, accountId, threadId].join(":"); } +function buildSyntheticApprovalRequest(routeRequest: ApprovalRouteRequest): ExecApprovalRequest { + return { + id: SYNTHETIC_APPROVAL_REQUEST_ID, + request: { + command: "", + agentId: routeRequest.agentId ?? null, + sessionKey: routeRequest.sessionKey ?? null, + turnSourceChannel: routeRequest.turnSourceChannel ?? null, + turnSourceTo: routeRequest.turnSourceTo ?? null, + turnSourceAccountId: routeRequest.turnSourceAccountId ?? null, + turnSourceThreadId: routeRequest.turnSourceThreadId ?? null, + }, + createdAtMs: 0, + expiresAtMs: 0, + }; +} + function shouldSkipForwardingFallback(params: { target: ExecApprovalForwardTarget; cfg: OpenClawConfig; - request: ExecApprovalRequest; + routeRequest: ApprovalRouteRequest; }): boolean { const channel = normalizeMessageChannel(params.target.channel) ?? params.target.channel; if (!channel) { return false; } - const adapter = getChannelPlugin(channel)?.execApprovals; + const adapter = resolveChannelApprovalAdapter(getChannelPlugin(channel)); return ( adapter?.delivery?.shouldSuppressForwardingFallback?.({ cfg: params.cfg, target: params.target, - request: params.request, + request: buildSyntheticApprovalRequest(params.routeRequest), }) ?? false ); } @@ -270,54 +334,112 @@ async function deliverToTargets(params: { await Promise.allSettled(deliveries); } -function buildRequestPayloadForTarget( - cfg: OpenClawConfig, - request: ExecApprovalRequest, - nowMsValue: number, - target: ForwardTarget, -): ReplyPayload { - const channel = normalizeMessageChannel(target.channel) ?? target.channel; +function buildExecPendingPayload(params: { + cfg: OpenClawConfig; + request: ExecApprovalRequest; + target: ForwardTarget; + nowMs: number; +}): ReplyPayload { + const channel = normalizeMessageChannel(params.target.channel) ?? params.target.channel; const pluginPayload = channel - ? getChannelPlugin(channel)?.execApprovals?.render?.exec?.buildPendingPayload?.({ - cfg, - request, - target, - nowMs: nowMsValue, - }) + ? resolveChannelApprovalAdapter(getChannelPlugin(channel))?.render?.exec?.buildPendingPayload?.( + { + cfg: params.cfg, + request: params.request, + target: params.target, + nowMs: params.nowMs, + }, + ) : null; if (pluginPayload) { return pluginPayload; } return buildApprovalPendingReplyPayload({ - approvalId: request.id, - approvalSlug: request.id.slice(0, 8), - text: buildRequestMessage(request, nowMsValue), + approvalId: params.request.id, + approvalSlug: params.request.id.slice(0, 8), + text: buildRequestMessage(params.request, params.nowMs), }); } -function buildResolvedPayloadForTarget( - cfg: OpenClawConfig, - resolved: ExecApprovalResolved, - target: ForwardTarget, -): ReplyPayload { - const channel = normalizeMessageChannel(target.channel) ?? target.channel; +function buildExecResolvedPayload(params: { + cfg: OpenClawConfig; + resolved: ExecApprovalResolved; + target: ForwardTarget; +}): ReplyPayload { + const channel = normalizeMessageChannel(params.target.channel) ?? params.target.channel; const pluginPayload = channel - ? getChannelPlugin(channel)?.execApprovals?.render?.exec?.buildResolvedPayload?.({ - cfg, - resolved, - target, + ? resolveChannelApprovalAdapter( + getChannelPlugin(channel), + )?.render?.exec?.buildResolvedPayload?.({ + cfg: params.cfg, + resolved: params.resolved, + target: params.target, }) : null; if (pluginPayload) { return pluginPayload; } - return { text: buildResolvedMessage(resolved) }; + return buildApprovalResolvedReplyPayload({ + approvalId: params.resolved.id, + approvalSlug: params.resolved.id.slice(0, 8), + text: buildResolvedMessage(params.resolved), + }); +} + +function buildPluginPendingPayload(params: { + cfg: OpenClawConfig; + request: PluginApprovalRequest; + target: ForwardTarget; + nowMs: number; +}): ReplyPayload { + const channel = normalizeMessageChannel(params.target.channel) ?? params.target.channel; + const adapterPayload = channel + ? resolveChannelApprovalAdapter( + getChannelPlugin(channel), + )?.render?.plugin?.buildPendingPayload?.({ + cfg: params.cfg, + request: params.request, + target: params.target, + nowMs: params.nowMs, + }) + : null; + if (adapterPayload) { + return adapterPayload; + } + return buildPluginApprovalPendingReplyPayload({ + request: params.request, + nowMs: params.nowMs, + text: buildPluginApprovalRequestMessage(params.request, params.nowMs), + }); +} + +function buildPluginResolvedPayload(params: { + cfg: OpenClawConfig; + resolved: PluginApprovalResolved; + target: ForwardTarget; +}): ReplyPayload { + const channel = normalizeMessageChannel(params.target.channel) ?? params.target.channel; + const adapterPayload = channel + ? resolveChannelApprovalAdapter( + getChannelPlugin(channel), + )?.render?.plugin?.buildResolvedPayload?.({ + cfg: params.cfg, + resolved: params.resolved, + target: params.target, + }) + : null; + if (adapterPayload) { + return adapterPayload; + } + return buildPluginApprovalResolvedReplyPayload({ + resolved: params.resolved, + }); } function resolveForwardTargets(params: { cfg: OpenClawConfig; config?: ExecApprovalForwardingConfig; - request: ExecApprovalRequest; + routeRequest: ApprovalRouteRequest; resolveSessionTarget: (params: { cfg: OpenClawConfig; request: ExecApprovalRequest; @@ -330,7 +452,7 @@ function resolveForwardTargets(params: { if (mode === "session" || mode === "both") { const sessionTarget = params.resolveSessionTarget({ cfg: params.cfg, - request: params.request, + request: buildSyntheticApprovalRequest(params.routeRequest), }); if (sessionTarget) { const key = buildTargetKey(sessionTarget); @@ -356,119 +478,151 @@ function resolveForwardTargets(params: { return targets; } -export function createExecApprovalForwarder( - deps: ExecApprovalForwarderDeps = {}, -): ExecApprovalForwarder { - const getConfig = deps.getConfig ?? loadConfig; - const deliver = deps.deliver ?? deliverOutboundPayloads; - const nowMs = deps.nowMs ?? Date.now; - const resolveSessionTarget = deps.resolveSessionTarget ?? defaultResolveSessionTarget; - const pending = new Map(); +function createApprovalHandlers< + TRequest, + TResolved, + TRouteRequest extends ApprovalRouteRequest = ApprovalRouteRequest, +>(params: { + strategy: ApprovalStrategy; + getConfig: () => OpenClawConfig; + deliver: typeof deliverOutboundPayloads; + nowMs: () => number; + resolveSessionTarget: (params: { + cfg: OpenClawConfig; + request: ExecApprovalRequest; + }) => ExecApprovalForwardTarget | null; +}) { + const pending = new Map>(); - const handleRequested = async (request: ExecApprovalRequest): Promise => { - const cfg = getConfig(); - const config = cfg.approvals?.exec; + const handleRequested = async (request: TRequest): Promise => { + const cfg = params.getConfig(); + const config = params.strategy.config(cfg); + const requestId = params.strategy.getRequestId(request); + const routeRequest = params.strategy.getRouteRequestFromRequest(request); const filteredTargets = [ - ...(shouldForward({ config, request }) + ...(shouldForwardRoute({ config, routeRequest }) ? resolveForwardTargets({ cfg, config, - request, - resolveSessionTarget, + routeRequest, + resolveSessionTarget: params.resolveSessionTarget, }) : []), - ].filter((target) => !shouldSkipForwardingFallback({ target, cfg, request })); + ].filter((target) => !shouldSkipForwardingFallback({ target, cfg, routeRequest })); if (filteredTargets.length === 0) { return false; } - const expiresInMs = Math.max(0, request.expiresAtMs - nowMs()); + const expiresInMs = Math.max(0, params.strategy.getExpiresAtMs(request) - params.nowMs()); const timeoutId = setTimeout(() => { void (async () => { - const entry = pending.get(request.id); + const entry = pending.get(requestId); if (!entry) { return; } - pending.delete(request.id); - const expiredText = buildExpiredMessage(request); + pending.delete(requestId); await deliverToTargets({ cfg, targets: entry.targets, - buildPayload: () => ({ text: expiredText }), - deliver, + buildPayload: () => ({ text: params.strategy.buildExpiredText(request) }), + deliver: params.deliver, }); })(); }, expiresInMs); timeoutId.unref?.(); - const pendingEntry: PendingApproval = { request, targets: filteredTargets, timeoutId }; - pending.set(request.id, pendingEntry); + const pendingEntry: PendingApproval = { + routeRequest, + targets: filteredTargets, + timeoutId, + }; + pending.set(requestId, pendingEntry); - if (pending.get(request.id) !== pendingEntry) { + if (pending.get(requestId) !== pendingEntry) { return false; } + void deliverToTargets({ cfg, targets: filteredTargets, - buildPayload: (target) => buildRequestPayloadForTarget(cfg, request, nowMs(), target), + buildPayload: (target) => + params.strategy.buildPendingPayload({ + cfg, + request, + target, + routeRequest, + nowMs: params.nowMs(), + }), beforeDeliver: async (target, payload) => { const channel = normalizeMessageChannel(target.channel) ?? target.channel; if (!channel) { return; } - await getChannelPlugin(channel)?.execApprovals?.delivery?.beforeDeliverPending?.({ + await resolveChannelApprovalAdapter( + getChannelPlugin(channel), + )?.delivery?.beforeDeliverPending?.({ cfg, target, payload, }); }, - deliver, - shouldSend: () => pending.get(request.id) === pendingEntry, + deliver: params.deliver, + shouldSend: () => pending.get(requestId) === pendingEntry, }).catch((err) => { - log.error(`exec approvals: failed to deliver request ${request.id}: ${String(err)}`); + log.error( + `${params.strategy.kind} approvals: failed to deliver request ${requestId}: ${String(err)}`, + ); }); return true; }; - const handleResolved = async (resolved: ExecApprovalResolved) => { - const entry = pending.get(resolved.id); + const handleResolved = async (resolved: TResolved) => { + const resolvedId = params.strategy.getResolvedId(resolved); + const entry = pending.get(resolvedId); + if (entry?.timeoutId) { + clearTimeout(entry.timeoutId); + } if (entry) { - if (entry.timeoutId) { - clearTimeout(entry.timeoutId); - } - pending.delete(resolved.id); + pending.delete(resolvedId); } - const cfg = getConfig(); - let targets = entry?.targets; - if (!targets && resolved.request) { - const request: ExecApprovalRequest = { - id: resolved.id, - request: resolved.request, - createdAtMs: resolved.ts, - expiresAtMs: resolved.ts, - }; - const config = cfg.approvals?.exec; - targets = [ - ...(shouldForward({ config, request }) - ? resolveForwardTargets({ - cfg, - config, - request, - resolveSessionTarget, - }) - : []), - ].filter((target) => !shouldSkipForwardingFallback({ target, cfg, request })); + const cfg = params.getConfig(); + let targets = entry?.targets; + if (!targets) { + const routeRequest = params.strategy.getRouteRequestFromResolved(resolved); + if (routeRequest) { + const config = params.strategy.config(cfg); + targets = [ + ...(shouldForwardRoute({ config, routeRequest }) + ? resolveForwardTargets({ + cfg, + config, + routeRequest, + resolveSessionTarget: params.resolveSessionTarget, + }) + : []), + ].filter((target) => !shouldSkipForwardingFallback({ target, cfg, routeRequest })); + } } - if (!targets || targets.length === 0) { + if (!targets?.length) { return; } + await deliverToTargets({ cfg, targets, - buildPayload: (target) => buildResolvedPayloadForTarget(cfg, resolved, target), - deliver, + buildPayload: (target) => + params.strategy.buildResolvedPayload({ + cfg, + resolved, + target, + routeRequest: + entry?.routeRequest ?? + params.strategy.getRouteRequestFromResolved(resolved) ?? + ({} as TRouteRequest), + }), + deliver: params.deliver, }); }; @@ -481,192 +635,123 @@ export function createExecApprovalForwarder( pending.clear(); }; - const toSyntheticExecRequestFromPlugin = (params: { - id: string; - request: PluginApprovalRequest["request"]; - createdAtMs: number; - expiresAtMs: number; - }): ExecApprovalRequest => ({ - id: params.id, - request: { - command: params.request.title, - agentId: params.request.agentId ?? null, - sessionKey: params.request.sessionKey ?? null, - turnSourceChannel: params.request.turnSourceChannel ?? null, - turnSourceTo: params.request.turnSourceTo ?? null, - turnSourceAccountId: params.request.turnSourceAccountId ?? null, - turnSourceThreadId: params.request.turnSourceThreadId ?? null, - }, - createdAtMs: params.createdAtMs, - expiresAtMs: params.expiresAtMs, + return { handleRequested, handleResolved, stop }; +} + +const execApprovalStrategy: ApprovalStrategy = { + kind: "exec", + config: (cfg) => cfg.approvals?.exec, + getRequestId: (request) => request.id, + getResolvedId: (resolved) => resolved.id, + getExpiresAtMs: (request) => request.expiresAtMs, + getRouteRequestFromRequest: (request) => ({ + agentId: request.request.agentId ?? null, + sessionKey: request.request.sessionKey ?? null, + turnSourceChannel: request.request.turnSourceChannel ?? null, + turnSourceTo: request.request.turnSourceTo ?? null, + turnSourceAccountId: request.request.turnSourceAccountId ?? null, + turnSourceThreadId: request.request.turnSourceThreadId ?? null, + }), + getRouteRequestFromResolved: (resolved) => + resolved.request + ? { + agentId: resolved.request.agentId ?? null, + sessionKey: resolved.request.sessionKey ?? null, + turnSourceChannel: resolved.request.turnSourceChannel ?? null, + turnSourceTo: resolved.request.turnSourceTo ?? null, + turnSourceAccountId: resolved.request.turnSourceAccountId ?? null, + turnSourceThreadId: resolved.request.turnSourceThreadId ?? null, + } + : null, + buildExpiredText: buildExpiredMessage, + buildPendingPayload: ({ cfg, request, target, nowMs }) => + buildExecPendingPayload({ + cfg, + request, + target, + nowMs, + }), + buildResolvedPayload: ({ cfg, resolved, target }) => + buildExecResolvedPayload({ + cfg, + resolved, + target, + }), +}; + +const pluginApprovalStrategy: ApprovalStrategy = { + kind: "plugin", + config: (cfg) => cfg.approvals?.plugin, + getRequestId: (request) => request.id, + getResolvedId: (resolved) => resolved.id, + getExpiresAtMs: (request) => request.expiresAtMs, + getRouteRequestFromRequest: (request) => ({ + agentId: request.request.agentId ?? null, + sessionKey: request.request.sessionKey ?? null, + turnSourceChannel: request.request.turnSourceChannel ?? null, + turnSourceTo: request.request.turnSourceTo ?? null, + turnSourceAccountId: request.request.turnSourceAccountId ?? null, + turnSourceThreadId: request.request.turnSourceThreadId ?? null, + }), + getRouteRequestFromResolved: (resolved) => + resolved.request + ? { + agentId: resolved.request.agentId ?? null, + sessionKey: resolved.request.sessionKey ?? null, + turnSourceChannel: resolved.request.turnSourceChannel ?? null, + turnSourceTo: resolved.request.turnSourceTo ?? null, + turnSourceAccountId: resolved.request.turnSourceAccountId ?? null, + turnSourceThreadId: resolved.request.turnSourceThreadId ?? null, + } + : null, + buildExpiredText: buildPluginApprovalExpiredMessage, + buildPendingPayload: ({ cfg, request, target, nowMs }) => + buildPluginPendingPayload({ + cfg, + request, + target, + nowMs, + }), + buildResolvedPayload: ({ cfg, resolved, target }) => + buildPluginResolvedPayload({ + cfg, + resolved, + target, + }), +}; + +export function createExecApprovalForwarder( + deps: ExecApprovalForwarderDeps = {}, +): ExecApprovalForwarder { + const getConfig = deps.getConfig ?? loadConfig; + const deliver = deps.deliver ?? deliverOutboundPayloads; + const nowMs = deps.nowMs ?? Date.now; + const resolveSessionTarget = deps.resolveSessionTarget ?? defaultResolveSessionTarget; + + const execHandlers = createApprovalHandlers({ + strategy: execApprovalStrategy, + getConfig, + deliver, + nowMs, + resolveSessionTarget, + }); + const pluginHandlers = createApprovalHandlers({ + strategy: pluginApprovalStrategy, + getConfig, + deliver, + nowMs, + resolveSessionTarget, }); - const pluginPending = new Map(); - - const handlePluginApprovalRequested = async ( - request: PluginApprovalRequest, - ): Promise => { - const cfg = getConfig(); - const config = cfg.approvals?.plugin; - const syntheticExecRequest = toSyntheticExecRequestFromPlugin({ - id: request.id, - request: request.request, - createdAtMs: request.createdAtMs, - expiresAtMs: request.expiresAtMs, - }); - - const filteredTargets = [ - ...(shouldForward({ config, request: syntheticExecRequest }) - ? resolveForwardTargets({ - cfg, - config, - request: syntheticExecRequest, - resolveSessionTarget, - }) - : []), - ].filter( - (target) => !shouldSkipForwardingFallback({ target, cfg, request: syntheticExecRequest }), - ); - - if (filteredTargets.length === 0) { - return false; - } - - const expiresInMs = Math.max(0, request.expiresAtMs - nowMs()); - const timeoutId = setTimeout(() => { - void (async () => { - const entry = pluginPending.get(request.id); - if (!entry) { - return; - } - pluginPending.delete(request.id); - const expiredText = buildPluginApprovalExpiredMessage(request); - await deliverToTargets({ - cfg, - targets: entry.targets, - buildPayload: () => ({ text: expiredText }), - deliver, - }); - })(); - }, expiresInMs); - timeoutId.unref?.(); - - const pendingEntry: PendingApproval = { - request: syntheticExecRequest, - targets: filteredTargets, - timeoutId, - }; - pluginPending.set(request.id, pendingEntry); - - void deliverToTargets({ - cfg, - targets: filteredTargets, - buildPayload: (target) => { - const channel = normalizeMessageChannel(target.channel) ?? target.channel; - const adapterPayload = channel - ? getChannelPlugin(channel)?.execApprovals?.render?.plugin?.buildPendingPayload?.({ - cfg, - request, - target, - nowMs: nowMs(), - }) - : null; - return ( - adapterPayload ?? - buildPluginApprovalPendingReplyPayload({ - request, - nowMs: nowMs(), - text: buildPluginApprovalRequestMessage(request, nowMs()), - }) - ); - }, - beforeDeliver: async (target, payload) => { - const channel = normalizeMessageChannel(target.channel) ?? target.channel; - if (!channel) { - return; - } - await getChannelPlugin(channel)?.execApprovals?.delivery?.beforeDeliverPending?.({ - cfg, - target, - payload, - }); - }, - deliver, - shouldSend: () => pluginPending.get(request.id) === pendingEntry, - }).catch((err) => { - log.error(`plugin approvals: failed to deliver request ${request.id}: ${String(err)}`); - }); - return true; - }; - - const handlePluginApprovalResolved = async (resolved: PluginApprovalResolved) => { - const cfg = getConfig(); - const entry = pluginPending.get(resolved.id); - if (entry) { - if (entry.timeoutId) { - clearTimeout(entry.timeoutId); - } - pluginPending.delete(resolved.id); - } - let targets = entry?.targets; - if (!targets && resolved.request) { - const syntheticExecRequest = toSyntheticExecRequestFromPlugin({ - id: resolved.id, - request: resolved.request, - createdAtMs: resolved.ts, - expiresAtMs: resolved.ts, - }); - const config = cfg.approvals?.plugin; - targets = [ - ...(shouldForward({ config, request: syntheticExecRequest }) - ? resolveForwardTargets({ - cfg, - config, - request: syntheticExecRequest, - resolveSessionTarget, - }) - : []), - ].filter( - (target) => !shouldSkipForwardingFallback({ target, cfg, request: syntheticExecRequest }), - ); - } - if (!targets || targets.length === 0) { - return; - } - await deliverToTargets({ - cfg, - targets, - buildPayload: (target) => { - const channel = normalizeMessageChannel(target.channel) ?? target.channel; - const adapterPayload = channel - ? getChannelPlugin(channel)?.execApprovals?.render?.plugin?.buildResolvedPayload?.({ - cfg, - resolved, - target, - }) - : null; - return adapterPayload ?? { text: buildPluginApprovalResolvedMessage(resolved) }; - }, - deliver, - }); - }; - - const stopAll = () => { - stop(); - for (const entry of pluginPending.values()) { - if (entry.timeoutId) { - clearTimeout(entry.timeoutId); - } - } - pluginPending.clear(); - }; - return { - handleRequested, - handleResolved, - handlePluginApprovalRequested, - handlePluginApprovalResolved, - stop: stopAll, + handleRequested: execHandlers.handleRequested, + handleResolved: execHandlers.handleResolved, + handlePluginApprovalRequested: pluginHandlers.handleRequested, + handlePluginApprovalResolved: pluginHandlers.handleResolved, + stop: () => { + execHandlers.stop(); + pluginHandlers.stop(); + }, }; } @@ -674,5 +759,8 @@ export function shouldForwardExecApproval(params: { config?: ExecApprovalForwardingConfig; request: ExecApprovalRequest; }): boolean { - return shouldForward(params); + return shouldForwardRoute({ + config: params.config, + routeRequest: execApprovalStrategy.getRouteRequestFromRequest(params.request), + }); } diff --git a/src/infra/plugin-approval-forwarder.test.ts b/src/infra/plugin-approval-forwarder.test.ts index 9cb70dca7ed..a7628dea17e 100644 --- a/src/infra/plugin-approval-forwarder.test.ts +++ b/src/infra/plugin-approval-forwarder.test.ts @@ -177,10 +177,10 @@ describe("plugin approval forwarding", () => { const mockPayload = { text: "custom adapter payload" }; const adapterPlugin: Pick< ChannelPlugin, - "id" | "meta" | "capabilities" | "config" | "execApprovals" + "id" | "meta" | "capabilities" | "config" | "approvals" > = { ...createChannelTestPluginBase({ id: "slack" as ChannelPlugin["id"] }), - execApprovals: { + approvals: { render: { plugin: { buildPendingPayload: vi.fn().mockReturnValue(mockPayload), @@ -209,10 +209,10 @@ describe("plugin approval forwarding", () => { const beforeDeliverPending = vi.fn(); const adapterPlugin: Pick< ChannelPlugin, - "id" | "meta" | "capabilities" | "config" | "execApprovals" + "id" | "meta" | "capabilities" | "config" | "approvals" > = { ...createChannelTestPluginBase({ id: "slack" as ChannelPlugin["id"] }), - execApprovals: { + approvals: { delivery: { beforeDeliverPending, }, @@ -236,10 +236,10 @@ describe("plugin approval forwarding", () => { const mockPayload = { text: "custom resolved payload" }; const adapterPlugin: Pick< ChannelPlugin, - "id" | "meta" | "capabilities" | "config" | "execApprovals" + "id" | "meta" | "capabilities" | "config" | "approvals" > = { ...createChannelTestPluginBase({ id: "slack" as ChannelPlugin["id"] }), - execApprovals: { + approvals: { render: { plugin: { buildResolvedPayload: vi.fn().mockReturnValue(mockPayload), diff --git a/src/plugin-sdk/approval-renderers.test.ts b/src/plugin-sdk/approval-renderers.test.ts index 0abbe604846..55a9ecdab88 100644 --- a/src/plugin-sdk/approval-renderers.test.ts +++ b/src/plugin-sdk/approval-renderers.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { buildApprovalPendingReplyPayload, + buildApprovalResolvedReplyPayload, buildPluginApprovalPendingReplyPayload, buildPluginApprovalResolvedReplyPayload, } from "./approval-renderers.js"; @@ -13,7 +14,7 @@ describe("plugin-sdk/approval-renderers", () => { text: "Approval required @everyone", }); - expect(payload.text).toContain("@\u200beveryone"); + expect(payload.text).toContain("@everyone"); expect(payload.interactive).toEqual({ blocks: [ { @@ -90,6 +91,7 @@ describe("plugin-sdk/approval-renderers", () => { approvalId: "plugin-approval-123", approvalSlug: "custom-slug", allowedDecisions: ["allow-once", "allow-always", "deny"], + state: "pending", }, telegram: { quoteText: "quoted", @@ -97,6 +99,23 @@ describe("plugin-sdk/approval-renderers", () => { }); }); + it("builds generic resolved payloads with approval metadata", () => { + const payload = buildApprovalResolvedReplyPayload({ + approvalId: "req-123", + approvalSlug: "req-123", + text: "resolved @everyone", + }); + + expect(payload.text).toBe("resolved @everyone"); + expect(payload.channelData).toEqual({ + execApproval: { + approvalId: "req-123", + approvalSlug: "req-123", + state: "resolved", + }, + }); + }); + it("builds plugin resolved payloads with optional channel data", () => { const payload = buildPluginApprovalResolvedReplyPayload({ resolved: { @@ -114,6 +133,11 @@ describe("plugin-sdk/approval-renderers", () => { expect(payload.text).toContain("Plugin approval allowed once"); expect(payload.channelData).toEqual({ + execApproval: { + approvalId: "plugin-approval-123", + approvalSlug: "plugin-a", + state: "resolved", + }, discord: { components: [{ type: "container" }], }, diff --git a/src/plugin-sdk/approval-renderers.ts b/src/plugin-sdk/approval-renderers.ts index 9aef26d7ee4..ca5be8eba58 100644 --- a/src/plugin-sdk/approval-renderers.ts +++ b/src/plugin-sdk/approval-renderers.ts @@ -12,14 +12,6 @@ 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; @@ -29,7 +21,7 @@ export function buildApprovalPendingReplyPayload(params: { }): ReplyPayload { const allowedDecisions = params.allowedDecisions ?? DEFAULT_ALLOWED_DECISIONS; return { - text: neutralizeApprovalText(params.text), + text: params.text, interactive: buildApprovalInteractiveReply({ approvalId: params.approvalId, allowedDecisions, @@ -39,6 +31,26 @@ export function buildApprovalPendingReplyPayload(params: { approvalId: params.approvalId, approvalSlug: params.approvalSlug, allowedDecisions, + state: "pending", + }, + ...params.channelData, + }, + }; +} + +export function buildApprovalResolvedReplyPayload(params: { + approvalId: string; + approvalSlug: string; + text: string; + channelData?: Record; +}): ReplyPayload { + return { + text: params.text, + channelData: { + execApproval: { + approvalId: params.approvalId, + approvalSlug: params.approvalSlug, + state: "resolved", }, ...params.channelData, }, @@ -65,18 +77,13 @@ export function buildPluginApprovalPendingReplyPayload(params: { export function buildPluginApprovalResolvedReplyPayload(params: { resolved: PluginApprovalResolved; text?: string; + approvalSlug?: string; channelData?: Record; }): ReplyPayload { - return params.channelData - ? { - text: neutralizeApprovalText( - params.text ?? buildPluginApprovalResolvedMessage(params.resolved), - ), - channelData: params.channelData, - } - : { - text: neutralizeApprovalText( - params.text ?? buildPluginApprovalResolvedMessage(params.resolved), - ), - }; + return buildApprovalResolvedReplyPayload({ + approvalId: params.resolved.id, + approvalSlug: params.approvalSlug ?? params.resolved.id.slice(0, 8), + text: params.text ?? buildPluginApprovalResolvedMessage(params.resolved), + channelData: params.channelData, + }); }