diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index c0758d7b315..96f5b1bf8b2 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -390,21 +390,32 @@ The config shape is identical to `approvals.exec`: `enabled`, `mode`, `agentFilt Channels that support interactive exec approval buttons (such as Telegram) also render buttons for plugin approvals. Channels without adapter support fall back to plain text with `/approve` instructions. -### Built-in chat approval clients +### Same-chat approvals on any channel -Discord and Telegram can also act as explicit exec approval clients with channel-specific config. +When an exec or plugin approval request originates from a deliverable chat surface, the same chat +can now approve it with `/approve` by default. This applies to channels such as Slack, Matrix, and +Microsoft Teams in addition to the existing Web UI and terminal UI flows. + +This shared text-command path uses the normal channel auth model for that conversation. If the +originating chat can already send commands and receive replies, approval requests no longer need a +separate channel-specific approval client just to stay pending. + +### Rich approval clients + +Discord and Telegram can also act as richer exec approval clients with channel-specific config. - Discord: `channels.discord.execApprovals.*` - Telegram: `channels.telegram.execApprovals.*` -These clients are opt-in. If a channel does not have exec approvals enabled, OpenClaw does not treat -that channel as an approval surface just because the conversation happened there. +These richer clients are opt-in. They add native DM routing, channel fanout, and interactive UI on +top of the shared same-chat `/approve` flow. Shared behavior: - only resolved approvers can approve or deny - Discord and Telegram approvers can be explicit (`execApprovals.approvers`) or inferred from existing owner config (`allowFrom`, plus direct-message `defaultTo` where supported) - the requester does not need to be an approver +- the originating chat can approve directly with `/approve` when that chat already supports commands and replies - when channel delivery is enabled, approval prompts include the command text - pending exec approvals expire after 30 minutes by default - if no operator UI or configured approval client can accept the request, the prompt falls back to `askFallback` diff --git a/extensions/discord/src/channel.ts b/extensions/discord/src/channel.ts index 0fa8e6b804b..4d8ef5420b5 100644 --- a/extensions/discord/src/channel.ts +++ b/extensions/discord/src/channel.ts @@ -492,10 +492,7 @@ export const discordPlugin: ChannelPlugin }, }, execApprovals: { - getInitiatingSurfaceState: ({ cfg, accountId }) => - isDiscordExecApprovalClientEnabled({ cfg, accountId }) - ? { kind: "enabled" } - : { kind: "disabled" }, + getInitiatingSurfaceState: () => ({ kind: "enabled" }), shouldSuppressLocalPrompt: ({ cfg, accountId, payload }) => shouldSuppressLocalDiscordExecApprovalPrompt({ cfg, diff --git a/extensions/telegram/src/channel.ts b/extensions/telegram/src/channel.ts index 8d53da05381..3e541db1311 100644 --- a/extensions/telegram/src/channel.ts +++ b/extensions/telegram/src/channel.ts @@ -457,10 +457,7 @@ export const telegramPlugin = createChatChannelPlugin({ }, }, execApprovals: { - getInitiatingSurfaceState: ({ cfg, accountId }) => - isTelegramExecApprovalClientEnabled({ cfg, accountId }) - ? { kind: "enabled" } - : { kind: "disabled" }, + getInitiatingSurfaceState: () => ({ kind: "enabled" }), hasConfiguredDmRoute: ({ cfg }) => hasTelegramExecApprovalDmRoute(cfg), shouldSuppressForwardingFallback: (params) => shouldSuppressTelegramExecApprovalForwardingFallback(params), diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index ad04ac60ad4..f6fd5b954fd 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -1,3 +1,4 @@ +import { hasApprovalTurnSourceRoute } from "../../infra/approval-turn-source.js"; import { sanitizeExecApprovalDisplayText } from "../../infra/exec-approval-command-display.js"; import type { ExecApprovalForwarder } from "../../infra/exec-approval-forwarder.js"; import { @@ -188,6 +189,9 @@ export function createExecApprovalHandlers( { dropIfSlow: true }, ); const hasExecApprovalClients = context.hasExecApprovalClients?.(client?.connId) ?? false; + const hasTurnSourceRoute = hasApprovalTurnSourceRoute({ + turnSourceChannel: record.request.turnSourceChannel, + }); let forwarded = false; if (opts?.forwarder) { try { @@ -202,7 +206,7 @@ export function createExecApprovalHandlers( } } - if (!hasExecApprovalClients && !forwarded) { + if (!hasExecApprovalClients && !forwarded && !hasTurnSourceRoute) { manager.expire(record.id, "no-approval-route"); respond( true, diff --git a/src/gateway/server-methods/plugin-approval.test.ts b/src/gateway/server-methods/plugin-approval.test.ts index f5c73cb7188..ab955cce78c 100644 --- a/src/gateway/server-methods/plugin-approval.test.ts +++ b/src/gateway/server-methods/plugin-approval.test.ts @@ -163,6 +163,52 @@ describe("createPluginApprovalHandlers", () => { expect(hasExecApprovalClients).toHaveBeenCalledWith("backend-conn-42"); }); + it("keeps plugin approvals pending when the originating chat can handle /approve directly", async () => { + vi.useFakeTimers(); + try { + const handlers = createPluginApprovalHandlers(manager); + const respond = vi.fn(); + const opts = createMockOptions( + "plugin.approval.request", + { + title: "Sensitive action", + description: "Desc", + twoPhase: true, + turnSourceChannel: "slack", + turnSourceTo: "C123", + }, + { + respond, + context: { + broadcast: vi.fn(), + logGateway: { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() }, + hasExecApprovalClients: () => false, + } as unknown as GatewayRequestHandlerOptions["context"], + }, + ); + + const requestPromise = handlers["plugin.approval.request"](opts); + + await vi.waitFor(() => { + expect(respond).toHaveBeenCalledWith( + true, + expect.objectContaining({ status: "accepted", id: expect.any(String) }), + undefined, + ); + }); + + const acceptedCall = respond.mock.calls.find( + (call) => (call[1] as Record)?.status === "accepted", + ); + const approvalId = (acceptedCall?.[1] as Record)?.id as string; + manager.resolve(approvalId, "allow-once"); + + await requestPromise; + } finally { + vi.useRealTimers(); + } + }); + it("rejects invalid severity value", async () => { const handlers = createPluginApprovalHandlers(manager); const opts = createMockOptions("plugin.approval.request", { diff --git a/src/gateway/server-methods/plugin-approval.ts b/src/gateway/server-methods/plugin-approval.ts index e8474a2ca8f..ba890909751 100644 --- a/src/gateway/server-methods/plugin-approval.ts +++ b/src/gateway/server-methods/plugin-approval.ts @@ -1,4 +1,5 @@ import { randomUUID } from "node:crypto"; +import { hasApprovalTurnSourceRoute } from "../../infra/approval-turn-source.js"; import type { ExecApprovalForwarder } from "../../infra/exec-approval-forwarder.js"; import type { ExecApprovalDecision } from "../../infra/exec-approvals.js"; import type { PluginApprovalRequestPayload } from "../../infra/plugin-approvals.js"; @@ -121,7 +122,10 @@ export function createPluginApprovalHandlers( } const hasApprovalClients = context.hasExecApprovalClients?.(client?.connId) ?? false; - if (!hasApprovalClients && !forwarded) { + const hasTurnSourceRoute = hasApprovalTurnSourceRoute({ + turnSourceChannel: record.request.turnSourceChannel, + }); + if (!hasApprovalClients && !forwarded && !hasTurnSourceRoute) { manager.expire(record.id, "no-approval-route"); respond( true, diff --git a/src/gateway/server-methods/server-methods.test.ts b/src/gateway/server-methods/server-methods.test.ts index d93a06c6c34..bf534cae23c 100644 --- a/src/gateway/server-methods/server-methods.test.ts +++ b/src/gateway/server-methods/server-methods.test.ts @@ -987,6 +987,45 @@ describe("exec approval handlers", () => { ); }); + it("keeps approvals pending when the originating chat can handle /approve directly", async () => { + vi.useFakeTimers(); + try { + const { manager, handlers, forwarder, respond, context } = + createForwardingExecApprovalFixture(); + const expireSpy = vi.spyOn(manager, "expire"); + + const requestPromise = requestExecApproval({ + handlers, + respond, + context, + params: { + twoPhase: true, + timeoutMs: 60_000, + id: "approval-chat-route", + host: "gateway", + turnSourceChannel: "slack", + turnSourceTo: "D123", + }, + }); + + await vi.waitFor(() => { + expect(respond).toHaveBeenCalledWith( + true, + expect.objectContaining({ status: "accepted", id: "approval-chat-route" }), + undefined, + ); + }); + + expect(forwarder.handleRequested).toHaveBeenCalledTimes(1); + expect(expireSpy).not.toHaveBeenCalled(); + + manager.resolve("approval-chat-route", "allow-once"); + await requestPromise; + } finally { + vi.useRealTimers(); + } + }); + it("keeps approvals pending when no approver clients but forwarding accepted the request", async () => { const { manager, handlers, forwarder, respond, context } = createForwardingExecApprovalFixture(); diff --git a/src/infra/approval-turn-source.test.ts b/src/infra/approval-turn-source.test.ts new file mode 100644 index 00000000000..9a2532e5350 --- /dev/null +++ b/src/infra/approval-turn-source.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from "vitest"; +import { hasApprovalTurnSourceRoute } from "./approval-turn-source.js"; + +describe("hasApprovalTurnSourceRoute", () => { + it("accepts operator UI turn sources", () => { + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "webchat" })).toBe(true); + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "tui" })).toBe(true); + }); + + it("accepts deliverable chat channels", () => { + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "slack" })).toBe(true); + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "discord" })).toBe(true); + }); + + it("rejects missing or unknown turn sources", () => { + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: undefined })).toBe(false); + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "unknown-channel" })).toBe(false); + }); +}); diff --git a/src/infra/approval-turn-source.ts b/src/infra/approval-turn-source.ts new file mode 100644 index 00000000000..a86016c4075 --- /dev/null +++ b/src/infra/approval-turn-source.ts @@ -0,0 +1,16 @@ +import { + INTERNAL_MESSAGE_CHANNEL, + isDeliverableMessageChannel, + normalizeMessageChannel, +} from "../utils/message-channel.js"; + +export function hasApprovalTurnSourceRoute(params: { turnSourceChannel?: string | null }): boolean { + const channel = normalizeMessageChannel(params.turnSourceChannel); + if (!channel) { + return false; + } + if (channel === INTERNAL_MESSAGE_CHANNEL || channel === "tui") { + return true; + } + return isDeliverableMessageChannel(channel); +} diff --git a/src/infra/exec-approval-surface.test.ts b/src/infra/exec-approval-surface.test.ts index 69f2959012f..643fbc37ef7 100644 --- a/src/infra/exec-approval-surface.test.ts +++ b/src/infra/exec-approval-surface.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const loadConfigMock = vi.hoisted(() => vi.fn()); const getChannelPluginMock = vi.hoisted(() => vi.fn()); const listChannelPluginsMock = vi.hoisted(() => vi.fn()); +const isDeliverableMessageChannelMock = vi.hoisted(() => vi.fn()); const normalizeMessageChannelMock = vi.hoisted(() => vi.fn()); type ExecApprovalSurfaceModule = typeof import("./exec-approval-surface.js"); @@ -15,10 +16,14 @@ async function loadExecApprovalSurfaceModule() { loadConfigMock.mockReset(); getChannelPluginMock.mockReset(); listChannelPluginsMock.mockReset(); + isDeliverableMessageChannelMock.mockReset(); normalizeMessageChannelMock.mockReset(); normalizeMessageChannelMock.mockImplementation((value?: string | null) => typeof value === "string" ? value.trim().toLowerCase() : undefined, ); + isDeliverableMessageChannelMock.mockImplementation( + (value?: string) => value === "slack" || value === "discord" || value === "telegram", + ); vi.doMock("../config/config.js", async (importOriginal) => { const actual = await importOriginal(); return { @@ -32,6 +37,7 @@ async function loadExecApprovalSurfaceModule() { })); vi.doMock("../utils/message-channel.js", () => ({ INTERNAL_MESSAGE_CHANNEL: "web", + isDeliverableMessageChannel: (...args: unknown[]) => isDeliverableMessageChannelMock(...args), normalizeMessageChannel: (...args: unknown[]) => normalizeMessageChannelMock(...args), })); ({ hasConfiguredExecApprovalDmRoute, resolveExecApprovalInitiatingSurfaceState } = @@ -146,6 +152,14 @@ describe("resolveExecApprovalInitiatingSurfaceState", () => { channelLabel: "Signal", }); }); + + it("treats deliverable chat channels without a custom adapter as enabled", () => { + expect(resolveExecApprovalInitiatingSurfaceState({ channel: "slack" })).toEqual({ + kind: "enabled", + channel: "slack", + channelLabel: "Slack", + }); + }); }); describe("hasConfiguredExecApprovalDmRoute", () => { diff --git a/src/infra/exec-approval-surface.ts b/src/infra/exec-approval-surface.ts index 2147baffe02..688df4d3ac3 100644 --- a/src/infra/exec-approval-surface.ts +++ b/src/infra/exec-approval-surface.ts @@ -1,6 +1,10 @@ import { getChannelPlugin, listChannelPlugins } from "../channels/plugins/index.js"; import { loadConfig, type OpenClawConfig } from "../config/config.js"; -import { INTERNAL_MESSAGE_CHANNEL, normalizeMessageChannel } from "../utils/message-channel.js"; +import { + INTERNAL_MESSAGE_CHANNEL, + isDeliverableMessageChannel, + normalizeMessageChannel, +} from "../utils/message-channel.js"; export type ExecApprovalInitiatingSurfaceState = | { kind: "enabled"; channel: string | undefined; channelLabel: string } @@ -41,6 +45,9 @@ export function resolveExecApprovalInitiatingSurfaceState(params: { if (state) { return { ...state, channel, channelLabel }; } + if (isDeliverableMessageChannel(channel)) { + return { kind: "enabled", channel, channelLabel }; + } return { kind: "unsupported", channel, channelLabel }; }