fix(approvals): restore native DM approval behavior

This commit is contained in:
Vincent Koc 2026-04-01 05:57:12 +09:00
parent fdad8ea3b0
commit f85aba43a9
9 changed files with 683 additions and 10 deletions

View File

@ -45,6 +45,7 @@ import {
listSlackDirectoryGroupsFromConfig,
listSlackDirectoryPeersFromConfig,
} from "./directory-config.js";
import { shouldSuppressLocalSlackExecApprovalPrompt } from "./exec-approvals.js";
import { resolveSlackGroupRequireMention, resolveSlackGroupToolPolicy } from "./group-policy.js";
import { isSlackInteractiveRepliesEnabled } from "./interactive-replies.js";
import { SLACK_TEXT_LIMIT } from "./limits.js";
@ -520,6 +521,12 @@ export const slackPlugin: ChannelPlugin<ResolvedSlackAccount, SlackProbe> = crea
deliveryMode: "direct",
chunker: null,
textChunkLimit: SLACK_TEXT_LIMIT,
shouldSuppressLocalPayloadPrompt: ({ cfg, accountId, payload }) =>
shouldSuppressLocalSlackExecApprovalPrompt({
cfg,
accountId,
payload,
}),
sendPayload: async (ctx) => {
const { send, tokenOverride } = resolveSlackSendContext({
cfg: ctx.cfg,

View File

@ -126,7 +126,7 @@ describe("slack exec approvals", () => {
cfg: buildConfig({ enabled: true, approvers: ["U123"] }),
payload,
}),
).toBe(false);
).toBe(true);
expect(
shouldSuppressLocalSlackExecApprovalPrompt({

View File

@ -1,5 +1,6 @@
import {
doesApprovalRequestMatchChannelAccount,
getExecApprovalReplyMetadata,
matchesApprovalRequestFilters,
resolveApprovalApprovers,
} from "openclaw/plugin-sdk/approval-runtime";
@ -144,10 +145,8 @@ export function shouldSuppressLocalSlackExecApprovalPrompt(params: {
accountId?: string | null;
payload: ReplyPayload;
}): boolean {
void params;
// Slack still uses the generic local pending-reply path. Unlike Discord and
// Telegram, there is no Slack runtime handler that sends a replacement native
// approval prompt via resolveChannelNativeApprovalDeliveryPlan, so suppressing
// the local payload can hide the only visible approval prompt.
return false;
return (
isSlackExecApprovalClientEnabled(params) &&
getExecApprovalReplyMetadata(params.payload) !== null
);
}

View File

@ -0,0 +1,162 @@
import type { App } from "@slack/bolt";
import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime";
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
const sendMessageSlackMock = vi.hoisted(() => vi.fn());
vi.mock("../send.js", () => ({
sendMessageSlack: sendMessageSlackMock,
}));
let SlackExecApprovalHandler: typeof import("./exec-approvals.js").SlackExecApprovalHandler;
function buildConfig(target: "dm" | "channel" | "both" = "dm"): OpenClawConfig {
return {
channels: {
slack: {
botToken: "xoxb-test",
appToken: "xapp-test",
execApprovals: {
enabled: true,
approvers: ["U123APPROVER"],
target,
},
},
},
} as OpenClawConfig;
}
function buildApp(): App {
return {
client: {
chat: {
update: vi.fn().mockResolvedValue(undefined),
},
},
} as unknown as App;
}
function buildRequest(overrides?: Partial<Record<string, unknown>>) {
return {
id: "req-1",
request: {
command: "python3 -c \"print('slack exec approval smoke')\"",
turnSourceChannel: "slack",
turnSourceTo: "channel:C123ROOM",
turnSourceAccountId: "default",
turnSourceThreadId: "1712345678.123456",
sessionKey: "agent:main:slack:channel:c123room:thread:1712345678.123456",
...overrides,
},
createdAtMs: 0,
expiresAtMs: Date.now() + 60_000,
};
}
describe("SlackExecApprovalHandler", () => {
beforeAll(async () => {
({ SlackExecApprovalHandler } = await import("./exec-approvals.js"));
});
beforeEach(() => {
sendMessageSlackMock.mockReset();
sendMessageSlackMock.mockResolvedValue({
messageId: "1712345678.999999",
channelId: "D123APPROVER",
});
});
it("delivers DM-first approvals and only posts a short origin notice", async () => {
const app = buildApp();
const handler = new SlackExecApprovalHandler({
app,
accountId: "default",
config: buildConfig("dm").channels!.slack!.execApprovals!,
cfg: buildConfig("dm"),
});
await handler.handleApprovalRequested(buildRequest());
expect(sendMessageSlackMock).toHaveBeenCalledTimes(2);
expect(sendMessageSlackMock).toHaveBeenNthCalledWith(
1,
"channel:C123ROOM",
"Approval required. I sent approval DMs to the approvers for this account.",
expect.objectContaining({
accountId: "default",
threadTs: "1712345678.123456",
}),
);
expect(sendMessageSlackMock).toHaveBeenNthCalledWith(
2,
"user:U123APPROVER",
expect.stringContaining("Exec approval required"),
expect.objectContaining({
accountId: "default",
blocks: expect.arrayContaining([expect.objectContaining({ type: "actions" })]),
}),
);
});
it("does not post a redundant DM redirect notice when the origin is already the approver DM", async () => {
const app = buildApp();
const handler = new SlackExecApprovalHandler({
app,
accountId: "default",
config: buildConfig("dm").channels!.slack!.execApprovals!,
cfg: buildConfig("dm"),
});
await handler.handleApprovalRequested(
buildRequest({
turnSourceTo: "user:U123APPROVER",
turnSourceThreadId: undefined,
sessionKey: "agent:main:slack:direct:U123APPROVER",
}),
);
expect(sendMessageSlackMock).toHaveBeenCalledTimes(1);
expect(sendMessageSlackMock).toHaveBeenCalledWith(
"user:U123APPROVER",
expect.stringContaining("Exec approval required"),
expect.objectContaining({
blocks: expect.arrayContaining([expect.objectContaining({ type: "actions" })]),
}),
);
});
it("updates the pending approval card in place after resolution", async () => {
const app = buildApp();
const update = app.client.chat.update as ReturnType<typeof vi.fn>;
const handler = new SlackExecApprovalHandler({
app,
accountId: "default",
config: buildConfig("dm").channels!.slack!.execApprovals!,
cfg: buildConfig("dm"),
});
await handler.handleApprovalRequested(
buildRequest({
turnSourceTo: "user:U123APPROVER",
turnSourceThreadId: undefined,
sessionKey: "agent:main:slack:direct:U123APPROVER",
}),
);
await handler.handleApprovalResolved({
id: "req-1",
decision: "allow-once",
resolvedBy: "U123APPROVER",
request: buildRequest().request,
ts: Date.now(),
});
expect(update).toHaveBeenCalledWith(
expect.objectContaining({
channel: "D123APPROVER",
ts: "1712345678.999999",
text: expect.stringContaining("Exec approval: Allowed once"),
blocks: expect.not.arrayContaining([expect.objectContaining({ type: "actions" })]),
}),
);
});
});

View File

@ -0,0 +1,408 @@
import type { App } from "@slack/bolt";
import type { Block, KnownBlock } from "@slack/web-api";
import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime";
import {
buildApprovalInteractiveReply,
createExecApprovalChannelRuntime,
getExecApprovalApproverDmNoticeText,
resolveChannelNativeApprovalDeliveryPlan,
resolveExecApprovalCommandDisplay,
type ExecApprovalChannelRuntime,
type ExecApprovalDecision,
type ExecApprovalRequest,
type ExecApprovalResolved,
} from "openclaw/plugin-sdk/infra-runtime";
import { logError } from "openclaw/plugin-sdk/text-runtime";
import { slackNativeApprovalAdapter } from "../approval-native.js";
import { getSlackExecApprovalApprovers, normalizeSlackApproverId } from "../exec-approvals.js";
import { resolveSlackReplyBlocks } from "../reply-blocks.js";
import { sendMessageSlack } from "../send.js";
type SlackBlock = Block | KnownBlock;
type SlackPendingApproval = {
channelId: string;
messageTs: string;
};
type SlackExecApprovalConfig = NonNullable<
NonNullable<NonNullable<OpenClawConfig["channels"]>["slack"]>["execApprovals"]
>;
type SlackExecApprovalHandlerOpts = {
app: App;
accountId: string;
config: SlackExecApprovalConfig;
gatewayUrl?: string;
cfg: OpenClawConfig;
};
function truncateSlackMrkdwn(text: string, maxChars: number): string {
return text.length <= maxChars ? text : `${text.slice(0, maxChars - 1)}`;
}
function buildSlackCodeBlock(text: string): string {
let fence = "```";
while (text.includes(fence)) {
fence += "`";
}
return `${fence}\n${text}\n${fence}`;
}
function formatSlackApprover(resolvedBy?: string | null): string | null {
const normalized = resolvedBy ? normalizeSlackApproverId(resolvedBy) : undefined;
if (normalized) {
return `<@${normalized}>`;
}
const trimmed = resolvedBy?.trim();
return trimmed ? trimmed : null;
}
function buildSlackApprovalContextLines(request: ExecApprovalRequest): string[] {
const lines: string[] = [];
if (request.request.agentId) {
lines.push(`*Agent:* ${request.request.agentId}`);
}
if (request.request.cwd) {
lines.push(`*CWD:* ${request.request.cwd}`);
}
if (request.request.host) {
lines.push(`*Host:* ${request.request.host}`);
}
return lines;
}
function buildSlackPendingApprovalText(request: ExecApprovalRequest): string {
const { commandText } = resolveExecApprovalCommandDisplay(request.request);
const lines = [
"*Exec approval required*",
"A command needs your approval.",
"",
"*Command*",
buildSlackCodeBlock(commandText),
...buildSlackApprovalContextLines(request),
];
return lines.join("\n");
}
function buildSlackPendingApprovalBlocks(request: ExecApprovalRequest): SlackBlock[] {
const { commandText } = resolveExecApprovalCommandDisplay(request.request);
const metadataLines = buildSlackApprovalContextLines(request);
const interactiveBlocks =
resolveSlackReplyBlocks({
text: "",
interactive: buildApprovalInteractiveReply({
approvalId: request.id,
}),
}) ?? [];
return [
{
type: "section",
text: {
type: "mrkdwn",
text: "*Exec approval required*\nA command needs your approval.",
},
},
{
type: "section",
text: {
type: "mrkdwn",
text: `*Command*\n${buildSlackCodeBlock(truncateSlackMrkdwn(commandText, 2600))}`,
},
},
...(metadataLines.length > 0
? [
{
type: "context",
elements: metadataLines.map((line) => ({
type: "mrkdwn" as const,
text: line,
})),
} satisfies SlackBlock,
]
: []),
...interactiveBlocks,
];
}
function buildSlackResolvedText(params: {
request: ExecApprovalRequest;
decision: ExecApprovalDecision;
resolvedBy?: string | null;
}): string {
const { commandText } = resolveExecApprovalCommandDisplay(params.request.request);
const decisionLabel =
params.decision === "allow-once"
? "Allowed once"
: params.decision === "allow-always"
? "Allowed always"
: "Denied";
const resolvedBy = formatSlackApprover(params.resolvedBy);
const lines = [
`*Exec approval: ${decisionLabel}*`,
resolvedBy ? `Resolved by ${resolvedBy}.` : "Resolved.",
"",
"*Command*",
buildSlackCodeBlock(commandText),
];
return lines.join("\n");
}
function buildSlackResolvedBlocks(params: {
request: ExecApprovalRequest;
decision: ExecApprovalDecision;
resolvedBy?: string | null;
}): SlackBlock[] {
const { commandText } = resolveExecApprovalCommandDisplay(params.request.request);
const decisionLabel =
params.decision === "allow-once"
? "Allowed once"
: params.decision === "allow-always"
? "Allowed always"
: "Denied";
const resolvedBy = formatSlackApprover(params.resolvedBy);
return [
{
type: "section",
text: {
type: "mrkdwn",
text: `*Exec approval: ${decisionLabel}*\n${resolvedBy ? `Resolved by ${resolvedBy}.` : "Resolved."}`,
},
},
{
type: "section",
text: {
type: "mrkdwn",
text: `*Command*\n${buildSlackCodeBlock(truncateSlackMrkdwn(commandText, 2600))}`,
},
},
];
}
function buildSlackExpiredText(request: ExecApprovalRequest): string {
const { commandText } = resolveExecApprovalCommandDisplay(request.request);
return [
"*Exec approval expired*",
"This approval request expired before it was resolved.",
"",
"*Command*",
buildSlackCodeBlock(commandText),
].join("\n");
}
function buildSlackExpiredBlocks(request: ExecApprovalRequest): SlackBlock[] {
const { commandText } = resolveExecApprovalCommandDisplay(request.request);
return [
{
type: "section",
text: {
type: "mrkdwn",
text: "*Exec approval expired*\nThis approval request expired before it was resolved.",
},
},
{
type: "section",
text: {
type: "mrkdwn",
text: `*Command*\n${buildSlackCodeBlock(truncateSlackMrkdwn(commandText, 2600))}`,
},
},
];
}
function buildDeliveryTargetKey(target: { to: string; threadId?: string | number | null }): string {
return `${target.to}:${target.threadId == null ? "" : String(target.threadId)}`;
}
export class SlackExecApprovalHandler {
private readonly runtime: ExecApprovalChannelRuntime<ExecApprovalRequest, ExecApprovalResolved>;
private readonly opts: SlackExecApprovalHandlerOpts;
constructor(opts: SlackExecApprovalHandlerOpts) {
this.opts = opts;
this.runtime = createExecApprovalChannelRuntime<SlackPendingApproval>({
label: "slack/exec-approvals",
clientDisplayName: "Slack Exec Approvals",
cfg: opts.cfg,
gatewayUrl: opts.gatewayUrl,
isConfigured: () =>
Boolean(
opts.config.enabled &&
getSlackExecApprovalApprovers({
cfg: opts.cfg,
accountId: opts.accountId,
}).length > 0,
),
shouldHandle: (request) => this.shouldHandle(request),
deliverRequested: async (request) => await this.deliverRequested(request),
finalizeResolved: async ({ request, resolved, entries }) => {
await this.finalizeResolved(request, resolved, entries);
},
finalizeExpired: async ({ request, entries }) => {
await this.finalizeExpired(request, entries);
},
});
}
shouldHandle(request: ExecApprovalRequest): boolean {
if (!this.opts.config.enabled) {
return false;
}
if ((this.opts.config.approvers?.length ?? 0) === 0) {
return false;
}
return (
slackNativeApprovalAdapter.native?.describeDeliveryCapabilities({
cfg: this.opts.cfg,
accountId: this.opts.accountId,
approvalKind: "exec",
request,
}).enabled === true
);
}
async start(): Promise<void> {
await this.runtime.start();
}
async stop(): Promise<void> {
await this.runtime.stop();
}
async handleApprovalRequested(request: ExecApprovalRequest): Promise<void> {
await this.runtime.handleRequested(request);
}
async handleApprovalResolved(resolved: ExecApprovalResolved): Promise<void> {
await this.runtime.handleResolved(resolved);
}
async handleApprovalTimeout(approvalId: string): Promise<void> {
await this.runtime.handleExpired(approvalId);
}
private async deliverRequested(request: ExecApprovalRequest): Promise<SlackPendingApproval[]> {
const deliveryPlan = await resolveChannelNativeApprovalDeliveryPlan({
cfg: this.opts.cfg,
accountId: this.opts.accountId,
approvalKind: "exec",
request,
adapter: slackNativeApprovalAdapter.native,
});
const pendingEntries: SlackPendingApproval[] = [];
const originTargetKey = deliveryPlan.originTarget
? buildDeliveryTargetKey(deliveryPlan.originTarget)
: null;
const targetKeys = new Set(
deliveryPlan.targets.map((target) => buildDeliveryTargetKey(target.target)),
);
if (
deliveryPlan.notifyOriginWhenDmOnly &&
deliveryPlan.originTarget &&
(originTargetKey == null || !targetKeys.has(originTargetKey))
) {
try {
await sendMessageSlack(
deliveryPlan.originTarget.to,
getExecApprovalApproverDmNoticeText(),
{
cfg: this.opts.cfg,
accountId: this.opts.accountId,
threadTs:
deliveryPlan.originTarget.threadId != null
? String(deliveryPlan.originTarget.threadId)
: undefined,
client: this.opts.app.client,
},
);
} catch (err) {
logError(`slack exec approvals: failed to send DM redirect notice: ${String(err)}`);
}
}
for (const deliveryTarget of deliveryPlan.targets) {
try {
const message = await sendMessageSlack(
deliveryTarget.target.to,
buildSlackPendingApprovalText(request),
{
cfg: this.opts.cfg,
accountId: this.opts.accountId,
threadTs:
deliveryTarget.target.threadId != null
? String(deliveryTarget.target.threadId)
: undefined,
blocks: buildSlackPendingApprovalBlocks(request),
client: this.opts.app.client,
},
);
pendingEntries.push({
channelId: message.channelId,
messageTs: message.messageId,
});
} catch (err) {
logError(`slack exec approvals: failed to deliver approval ${request.id}: ${String(err)}`);
}
}
return pendingEntries;
}
private async finalizeResolved(
request: ExecApprovalRequest,
resolved: ExecApprovalResolved,
entries: SlackPendingApproval[],
): Promise<void> {
const text = buildSlackResolvedText({
request,
decision: resolved.decision,
resolvedBy: resolved.resolvedBy,
});
const blocks = buildSlackResolvedBlocks({
request,
decision: resolved.decision,
resolvedBy: resolved.resolvedBy,
});
for (const entry of entries) {
await this.updateMessage({
channelId: entry.channelId,
messageTs: entry.messageTs,
text,
blocks,
});
}
}
private async finalizeExpired(
request: ExecApprovalRequest,
entries: SlackPendingApproval[],
): Promise<void> {
const blocks = buildSlackExpiredBlocks(request);
const text = buildSlackExpiredText(request);
for (const entry of entries) {
await this.updateMessage({
channelId: entry.channelId,
messageTs: entry.messageTs,
text,
blocks,
});
}
}
private async updateMessage(params: {
channelId: string;
messageTs: string;
text: string;
blocks: SlackBlock[];
}): Promise<void> {
try {
await this.opts.app.client.chat.update({
channel: params.channelId,
ts: params.messageTs,
text: params.text,
blocks: params.blocks,
});
} catch (err) {
logError(`slack exec approvals: failed to update message: ${String(err)}`);
}
}
}

View File

@ -40,6 +40,7 @@ import { normalizeAllowList } from "./allow-list.js";
import { resolveSlackSlashCommandConfig } from "./commands.js";
import { createSlackMonitorContext } from "./context.js";
import { registerSlackMonitorEvents } from "./events.js";
import { SlackExecApprovalHandler } from "./exec-approvals.js";
import { createSlackMessageHandler } from "./message-handler.js";
import {
formatUnknownError,
@ -405,9 +406,18 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) {
: undefined;
const handleSlackMessage = createSlackMessageHandler({ ctx, account, trackEvent });
const execApprovalsHandler = slackCfg.execApprovals?.enabled
? new SlackExecApprovalHandler({
app,
accountId: account.accountId,
config: slackCfg.execApprovals,
cfg,
})
: null;
registerSlackMonitorEvents({ ctx, account, handleSlackMessage, trackEvent });
await registerSlackMonitorSlashCommands({ ctx, account });
await execApprovalsHandler?.start();
if (slackMode === "http" && slackHttpHandler) {
unregisterHttpHandler = registerSlackHttpHandler({
path: slackWebhookPath,
@ -613,6 +623,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) {
} finally {
opts.abortSignal?.removeEventListener("abort", stopOnAbort);
unregisterHttpHandler?.();
await execApprovalsHandler?.stop().catch(() => undefined);
await app.stop().catch(() => undefined);
}
}

View File

@ -0,0 +1,48 @@
import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime";
import { describe, expect, it } from "vitest";
import { telegramNativeApprovalAdapter } from "./approval-native.js";
function buildConfig(
overrides?: Partial<NonNullable<NonNullable<OpenClawConfig["channels"]>["telegram"]>>,
): OpenClawConfig {
return {
channels: {
telegram: {
botToken: "tok",
execApprovals: {
enabled: true,
approvers: ["8460800771"],
target: "dm",
},
...overrides,
},
},
} as OpenClawConfig;
}
describe("telegram native approval adapter", () => {
it("normalizes direct-chat origin targets so DM dedupe can converge", async () => {
const target = await telegramNativeApprovalAdapter.native?.resolveOriginTarget?.({
cfg: buildConfig(),
accountId: "default",
approvalKind: "exec",
request: {
id: "req-1",
request: {
command: "echo hi",
turnSourceChannel: "telegram",
turnSourceTo: "telegram:8460800771",
turnSourceAccountId: "default",
sessionKey: "agent:main:telegram:direct:8460800771",
},
createdAtMs: 0,
expiresAtMs: 1000,
},
});
expect(target).toEqual({
to: "8460800771",
threadId: undefined,
});
});
});

View File

@ -15,6 +15,7 @@ import {
isTelegramExecApprovalClientEnabled,
resolveTelegramExecApprovalTarget,
} from "./exec-approvals.js";
import { normalizeTelegramChatId } from "./targets.js";
type ApprovalRequest = ExecApprovalRequest | PluginApprovalRequest;
type TelegramOriginTarget = { to: string; threadId?: number; accountId?: string };
@ -62,8 +63,9 @@ function resolveTurnSourceTelegramOriginTarget(params: {
request: ApprovalRequest;
}): TelegramOriginTarget | null {
const turnSourceChannel = params.request.request.turnSourceChannel?.trim().toLowerCase() || "";
const turnSourceTo = params.request.request.turnSourceTo?.trim() || "";
const rawTurnSourceTo = params.request.request.turnSourceTo?.trim() || "";
const turnSourceAccountId = params.request.request.turnSourceAccountId?.trim() || "";
const turnSourceTo = normalizeTelegramChatId(rawTurnSourceTo) ?? rawTurnSourceTo;
if (turnSourceChannel !== "telegram" || !turnSourceTo) {
return null;
}
@ -102,7 +104,7 @@ function resolveSessionTelegramOriginTarget(params: {
return null;
}
return {
to: sessionTarget.to,
to: normalizeTelegramChatId(sessionTarget.to) ?? sessionTarget.to,
threadId: sessionTarget.threadId,
accountId: sessionTarget.accountId,
};
@ -113,7 +115,9 @@ function telegramTargetsMatch(a: TelegramOriginTarget, b: TelegramOriginTarget):
!a.accountId ||
!b.accountId ||
normalizeAccountId(a.accountId) === normalizeAccountId(b.accountId);
return a.to === b.to && a.threadId === b.threadId && accountMatches;
const normalizedA = normalizeTelegramChatId(a.to) ?? a.to;
const normalizedB = normalizeTelegramChatId(b.to) ?? b.to;
return normalizedA === normalizedB && a.threadId === b.threadId && accountMatches;
}
function resolveTelegramOriginTarget(params: {

View File

@ -143,6 +143,40 @@ describe("TelegramExecApprovalHandler", () => {
expect(sendMessage.mock.calls.map((call) => call[0])).toEqual(["111", "222"]);
});
it("does not double-send in direct chats when the origin chat is the approver DM", async () => {
const cfg = {
channels: {
telegram: {
execApprovals: {
enabled: true,
approvers: ["8460800771"],
target: "dm",
},
},
},
} as OpenClawConfig;
const { handler, sendMessage } = createHandler(cfg);
await handler.handleRequested({
...baseRequest,
request: {
...baseRequest.request,
sessionKey: "agent:main:telegram:direct:8460800771",
turnSourceTo: "telegram:8460800771",
turnSourceThreadId: undefined,
},
});
expect(sendMessage).toHaveBeenCalledTimes(1);
expect(sendMessage).toHaveBeenCalledWith(
"8460800771",
expect.stringContaining("/approve 9f1c7d5d-b1fb-46ef-ac45-662723b65bb7 allow-once"),
expect.objectContaining({
accountId: "default",
}),
);
});
it("clears buttons from tracked approval messages when resolved", async () => {
const cfg = {
channels: {