From 3ec000b99554f186c9ad9e5c6db0bfd5dcf6b3f4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 30 Mar 2026 06:52:09 +0900 Subject: [PATCH] refactor: align same-chat approval routing --- docs/tools/exec-approvals.md | 7 ++- extensions/discord/src/channel.ts | 5 +- extensions/telegram/src/channel.ts | 6 +- .../telegram/src/exec-approvals.test.ts | 5 ++ extensions/telegram/src/exec-approvals.ts | 5 +- .../bash-tools.exec.approval-id.test.ts | 36 +++++------- src/auto-reply/reply/commands-approve.ts | 47 +++------------- src/auto-reply/reply/commands.test.ts | 56 +++++++++++++------ src/gateway/server-methods/exec-approval.ts | 1 + src/gateway/server-methods/plugin-approval.ts | 1 + src/infra/approval-turn-source.test.ts | 51 ++++++++++++++--- src/infra/approval-turn-source.ts | 26 +++++---- 12 files changed, 141 insertions(+), 105 deletions(-) diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index 96f5b1bf8b2..2af4de4cdf3 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -400,6 +400,9 @@ This shared text-command path uses the normal channel auth model for that conver originating chat can already send commands and receive replies, approval requests no longer need a separate channel-specific approval client just to stay pending. +Discord and Telegram also support same-chat `/approve`, but those channels still use their +resolved approver list for authorization even when the richer approval client is disabled. + ### Rich approval clients Discord and Telegram can also act as richer exec approval clients with channel-specific config. @@ -412,7 +415,9 @@ top of the shared same-chat `/approve` flow. Shared behavior: -- only resolved approvers can approve or deny +- Slack, Matrix, Microsoft Teams, and similar deliverable chats use the normal channel auth model + for same-chat `/approve` +- for Discord and Telegram, 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 diff --git a/extensions/discord/src/channel.ts b/extensions/discord/src/channel.ts index 4d8ef5420b5..5ab617d6436 100644 --- a/extensions/discord/src/channel.ts +++ b/extensions/discord/src/channel.ts @@ -492,7 +492,10 @@ export const discordPlugin: ChannelPlugin }, }, execApprovals: { - getInitiatingSurfaceState: () => ({ kind: "enabled" }), + getInitiatingSurfaceState: ({ cfg, accountId }) => + getDiscordExecApprovalApprovers({ cfg, accountId }).length > 0 + ? { kind: "enabled" } + : { kind: "disabled" }, shouldSuppressLocalPrompt: ({ cfg, accountId, payload }) => shouldSuppressLocalDiscordExecApprovalPrompt({ cfg, diff --git a/extensions/telegram/src/channel.ts b/extensions/telegram/src/channel.ts index 3e541db1311..236cc95209f 100644 --- a/extensions/telegram/src/channel.ts +++ b/extensions/telegram/src/channel.ts @@ -52,6 +52,7 @@ import { shouldSuppressTelegramExecApprovalForwardingFallback, } from "./exec-approval-forwarding.js"; import { + getTelegramExecApprovalApprovers, isTelegramExecApprovalClientEnabled, resolveTelegramExecApprovalTarget, } from "./exec-approvals.js"; @@ -457,7 +458,10 @@ export const telegramPlugin = createChatChannelPlugin({ }, }, execApprovals: { - getInitiatingSurfaceState: () => ({ kind: "enabled" }), + getInitiatingSurfaceState: ({ cfg, accountId }) => + getTelegramExecApprovalApprovers({ cfg, accountId }).length > 0 + ? { kind: "enabled" } + : { kind: "disabled" }, hasConfiguredDmRoute: ({ cfg }) => hasTelegramExecApprovalDmRoute(cfg), shouldSuppressForwardingFallback: (params) => shouldSuppressTelegramExecApprovalForwardingFallback(params), diff --git a/extensions/telegram/src/exec-approvals.test.ts b/extensions/telegram/src/exec-approvals.test.ts index f86f9ae6bdb..936cb51ea93 100644 --- a/extensions/telegram/src/exec-approvals.test.ts +++ b/extensions/telegram/src/exec-approvals.test.ts @@ -221,6 +221,11 @@ describe("telegram exec approvals", () => { expect(isTelegramExecApprovalAuthorizedSender({ cfg, senderId: "123" })).toBe(true); }); + it("accepts explicit approvers even when the richer client is disabled", () => { + const cfg = buildConfig({ enabled: false, approvers: ["123"] }); + expect(isTelegramExecApprovalAuthorizedSender({ cfg, senderId: "123" })).toBe(true); + }); + it("accepts active forwarded DM targets", () => { const cfg = { channels: { telegram: { botToken: "tok" } }, diff --git a/extensions/telegram/src/exec-approvals.ts b/extensions/telegram/src/exec-approvals.ts index 5efefb9f8d1..65da3a7303f 100644 --- a/extensions/telegram/src/exec-approvals.ts +++ b/extensions/telegram/src/exec-approvals.ts @@ -126,10 +126,7 @@ export function isTelegramExecApprovalAuthorizedSender(params: { accountId?: string | null; senderId?: string | null; }): boolean { - return ( - (isTelegramExecApprovalClientEnabled(params) && isTelegramExecApprovalApprover(params)) || - isTelegramExecApprovalTargetRecipient(params) - ); + return isTelegramExecApprovalApprover(params) || isTelegramExecApprovalTargetRecipient(params); } export function resolveTelegramExecApprovalTarget(params: { diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 546c79d964f..a7dcc8eaa6d 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -28,12 +28,14 @@ vi.mock("../infra/exec-obfuscation-detect.js", () => ({ let callGatewayTool: typeof import("./tools/gateway.js").callGatewayTool; let createExecTool: typeof import("./bash-tools.exec.js").createExecTool; let detectCommandObfuscation: typeof import("../infra/exec-obfuscation-detect.js").detectCommandObfuscation; +let getExecApprovalApproverDmNoticeText: typeof import("../infra/exec-approval-reply.js").getExecApprovalApproverDmNoticeText; async function loadExecApprovalModules() { vi.resetModules(); ({ callGatewayTool } = await import("./tools/gateway.js")); ({ createExecTool } = await import("./bash-tools.exec.js")); ({ detectCommandObfuscation } = await import("../infra/exec-obfuscation-detect.js")); + ({ getExecApprovalApproverDmNoticeText } = await import("../infra/exec-approval-reply.js")); } function buildPreparedSystemRunPayload(rawInvokeParams: unknown) { @@ -192,20 +194,6 @@ function mockPendingApprovalRegistration() { }); } -function expectApprovalUnavailableText(result: { - details: { status?: string }; - content: Array<{ type?: string; text?: string }>; -}) { - expect(result.details.status).toBe("approval-unavailable"); - const text = result.content.find((part) => part.type === "text")?.text ?? ""; - expect(text).not.toContain("/approve"); - expect(text).not.toContain("npm view diver name version description"); - expect(text).not.toContain("Pending command:"); - expect(text).not.toContain("Host:"); - expect(text).not.toContain("CWD:"); - return text; -} - describe("exec approvals", () => { let previousHome: string | undefined; let previousUserProfile: string | undefined; @@ -690,7 +678,7 @@ describe("exec approvals", () => { ); }); - it("returns an unavailable approval message instead of a local /approve prompt when discord exec approvals are disabled", async () => { + it("shows a local /approve prompt when discord exec approvals are disabled", async () => { await writeOpenClawConfig({ channels: { discord: { @@ -715,12 +703,13 @@ describe("exec approvals", () => { command: "npm view diver name version description", }); - const text = expectApprovalUnavailableText(result); - expect(text).toContain("Discord does not support chat exec approvals."); - expect(text).toContain("Web UI or terminal UI"); + expectPendingApprovalText(result, { + command: "npm view diver name version description", + host: "gateway", + }); }); - it("tells Telegram users that allowed approvers were DMed when Telegram approvals are disabled but Discord DM approvals are enabled", async () => { + it("keeps Telegram approvals in the initiating chat even when Discord DM approvals are also enabled", async () => { await writeOpenClawConfig( { channels: { @@ -752,9 +741,12 @@ describe("exec approvals", () => { command: "npm view diver name version description", }); - const text = expectApprovalUnavailableText(result); - expect(text).toContain("Telegram does not support chat exec approvals."); - expect(text).toContain("Web UI or terminal UI"); + const details = expectPendingApprovalText(result, { + command: "npm view diver name version description", + host: "gateway", + }); + expect(getResultText(result)).toContain(`/approve ${details.approvalSlug} allow-once`); + expect(getResultText(result)).not.toContain(getExecApprovalApproverDmNoticeText()); }); it("denies node obfuscated command when approval request times out", async () => { diff --git a/src/auto-reply/reply/commands-approve.ts b/src/auto-reply/reply/commands-approve.ts index 35183879f7b..4d949eea339 100644 --- a/src/auto-reply/reply/commands-approve.ts +++ b/src/auto-reply/reply/commands-approve.ts @@ -1,10 +1,7 @@ import { callGateway } from "../../gateway/call.js"; import { ErrorCodes } from "../../gateway/protocol/index.js"; import { logVerbose } from "../../globals.js"; -import { - isDiscordExecApprovalApprover, - isDiscordExecApprovalClientEnabled, -} from "../../plugin-sdk/discord-surface.js"; +import { isDiscordExecApprovalApprover } from "../../plugin-sdk/discord-surface.js"; import { isTelegramExecApprovalAuthorizedSender, isTelegramExecApprovalApprover, @@ -130,8 +127,6 @@ export const handleApproveCommand: CommandHandler = async (params, allowTextComm return { shouldContinue: false, reply: { text: parsed.error } }; } const isPluginId = parsed.id.startsWith("plugin:"); - let discordExecApprovalDeniedReply: { shouldContinue: false; reply: { text: string } } | null = - null; let isTelegramExplicitApprover = false; if (params.command.channel === "telegram") { @@ -158,19 +153,14 @@ export const handleApproveCommand: CommandHandler = async (params, allowTextComm } if (params.command.channel === "discord" && !isPluginId) { - const discordApproverContext = { - cfg: params.cfg, - accountId: params.ctx.AccountId, - senderId: params.command.senderId, - }; - if (!isDiscordExecApprovalClientEnabled(discordApproverContext)) { - discordExecApprovalDeniedReply = { - shouldContinue: false, - reply: { text: "❌ Discord exec approvals are not enabled for this bot account." }, - }; - } - if (!discordExecApprovalDeniedReply && !isDiscordExecApprovalApprover(discordApproverContext)) { - discordExecApprovalDeniedReply = { + if ( + !isDiscordExecApprovalApprover({ + cfg: params.cfg, + accountId: params.ctx.AccountId, + senderId: params.command.senderId, + }) + ) { + return { shouldContinue: false, reply: { text: "❌ You are not authorized to approve exec requests on Discord." }, }; @@ -227,25 +217,6 @@ export const handleApproveCommand: CommandHandler = async (params, allowTextComm }; } } else { - if (discordExecApprovalDeniedReply) { - // Preserve the legacy unprefixed plugin fallback on Discord even when - // exec approvals are unavailable to this sender. - try { - await callApprovalMethod("plugin.approval.resolve"); - } catch (pluginErr) { - if (isApprovalNotFoundError(pluginErr)) { - return discordExecApprovalDeniedReply; - } - return { - shouldContinue: false, - reply: { text: `❌ Failed to submit approval: ${String(pluginErr)}` }, - }; - } - return { - shouldContinue: false, - reply: { text: `✅ Approval ${parsed.decision} submitted for ${parsed.id}.` }, - }; - } try { await callApprovalMethod("exec.approval.resolve"); } catch (err) { diff --git a/src/auto-reply/reply/commands.test.ts b/src/auto-reply/reply/commands.test.ts index d3f1eaf3b6b..a27d2a0eb95 100644 --- a/src/auto-reply/reply/commands.test.ts +++ b/src/auto-reply/reply/commands.test.ts @@ -457,26 +457,29 @@ describe("/approve command", () => { it("requires configured Discord approvers for exec approvals", async () => { for (const testCase of [ { - name: "discord approvals disabled", + name: "discord no approver policy", cfg: createDiscordApproveCfg(null), senderId: "123", - expectedText: "Discord exec approvals are not enabled", - setup: () => - callGatewayMock.mockRejectedValue( - gatewayError("unknown or expired approval id", "APPROVAL_NOT_FOUND"), - ), - expectedGatewayCalls: 1, + expectedText: "not authorized to approve", + setup: undefined, + expectedGatewayCalls: 0, }, { name: "discord non approver", cfg: createDiscordApproveCfg({ enabled: true, approvers: ["999"], target: "channel" }), senderId: "123", expectedText: "not authorized to approve", - setup: () => - callGatewayMock.mockRejectedValue( - gatewayError("unknown or expired approval id", "APPROVAL_NOT_FOUND"), - ), + setup: undefined, + expectedGatewayCalls: 0, + }, + { + name: "discord approver with rich client disabled", + cfg: createDiscordApproveCfg({ enabled: false, approvers: ["123"], target: "channel" }), + senderId: "123", + expectedText: "Approval allow-once submitted", + setup: () => callGatewayMock.mockResolvedValue({ ok: true }), expectedGatewayCalls: 1, + expectedMethod: "exec.approval.resolve", }, { name: "discord approver", @@ -485,10 +488,11 @@ describe("/approve command", () => { expectedText: "Approval allow-once submitted", setup: () => callGatewayMock.mockResolvedValue({ ok: true }), expectedGatewayCalls: 1, + expectedMethod: "exec.approval.resolve", }, ] as const) { callGatewayMock.mockReset(); - testCase.setup(); + testCase.setup?.(); const params = buildParams("/approve abc12345 allow-once", testCase.cfg, { Provider: "discord", Surface: "discord", @@ -499,13 +503,10 @@ describe("/approve command", () => { expect(result.shouldContinue, testCase.name).toBe(false); expect(result.reply?.text, testCase.name).toContain(testCase.expectedText); expect(callGatewayMock, testCase.name).toHaveBeenCalledTimes(testCase.expectedGatewayCalls); - if (testCase.expectedGatewayCalls > 0) { + if ("expectedMethod" in testCase) { expect(callGatewayMock, testCase.name).toHaveBeenCalledWith( expect.objectContaining({ - method: - testCase.name === "discord approver" - ? "exec.approval.resolve" - : "plugin.approval.resolve", + method: testCase.expectedMethod, params: { id: "abc12345", decision: "allow-once" }, }), ); @@ -632,6 +633,19 @@ describe("/approve command", () => { expectedText: "not authorized to approve", expectGatewayCalls: 0, }, + { + name: "telegram approver with rich client disabled", + cfg: createTelegramApproveCfg({ enabled: false, approvers: ["123"], target: "dm" }), + commandBody: "/approve abc12345 allow-once", + ctx: { + Provider: "telegram", + Surface: "telegram", + SenderId: "123", + }, + setup: () => callGatewayMock.mockResolvedValue({ ok: true }), + expectedText: "Approval allow-once submitted", + expectGatewayCalls: 1, + }, { name: "non approver", cfg: createTelegramApproveCfg({ enabled: true, approvers: ["999"], target: "dm" }), @@ -654,6 +668,14 @@ describe("/approve command", () => { expect(result.shouldContinue, testCase.name).toBe(false); expect(result.reply?.text, testCase.name).toContain(testCase.expectedText); expect(callGatewayMock, testCase.name).toHaveBeenCalledTimes(testCase.expectGatewayCalls); + if (testCase.expectGatewayCalls > 0) { + expect(callGatewayMock, testCase.name).toHaveBeenCalledWith( + expect.objectContaining({ + method: "exec.approval.resolve", + params: { id: "abc12345", decision: "allow-once" }, + }), + ); + } } }); diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index f6fd5b954fd..a8252a8995a 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -191,6 +191,7 @@ export function createExecApprovalHandlers( const hasExecApprovalClients = context.hasExecApprovalClients?.(client?.connId) ?? false; const hasTurnSourceRoute = hasApprovalTurnSourceRoute({ turnSourceChannel: record.request.turnSourceChannel, + turnSourceAccountId: record.request.turnSourceAccountId, }); let forwarded = false; if (opts?.forwarder) { diff --git a/src/gateway/server-methods/plugin-approval.ts b/src/gateway/server-methods/plugin-approval.ts index ba890909751..2646563740e 100644 --- a/src/gateway/server-methods/plugin-approval.ts +++ b/src/gateway/server-methods/plugin-approval.ts @@ -124,6 +124,7 @@ export function createPluginApprovalHandlers( const hasApprovalClients = context.hasExecApprovalClients?.(client?.connId) ?? false; const hasTurnSourceRoute = hasApprovalTurnSourceRoute({ turnSourceChannel: record.request.turnSourceChannel, + turnSourceAccountId: record.request.turnSourceAccountId, }); if (!hasApprovalClients && !forwarded && !hasTurnSourceRoute) { manager.expire(record.id, "no-approval-route"); diff --git a/src/infra/approval-turn-source.test.ts b/src/infra/approval-turn-source.test.ts index 9a2532e5350..4a114f8822e 100644 --- a/src/infra/approval-turn-source.test.ts +++ b/src/infra/approval-turn-source.test.ts @@ -1,19 +1,52 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const loadConfigMock = vi.hoisted(() => vi.fn()); +const resolveExecApprovalInitiatingSurfaceStateMock = vi.hoisted(() => vi.fn()); + +vi.mock("../config/config.js", () => ({ + loadConfig: () => loadConfigMock(), +})); + +vi.mock("./exec-approval-surface.js", () => ({ + resolveExecApprovalInitiatingSurfaceState: (...args: unknown[]) => + resolveExecApprovalInitiatingSurfaceStateMock(...args), +})); + 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); + beforeEach(() => { + loadConfigMock.mockReset(); + resolveExecApprovalInitiatingSurfaceStateMock.mockReset(); + loadConfigMock.mockReturnValue({ loaded: true }); }); - it("accepts deliverable chat channels", () => { - expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "slack" })).toBe(true); - expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "discord" })).toBe(true); + it("returns true when the initiating surface is enabled", () => { + resolveExecApprovalInitiatingSurfaceStateMock.mockReturnValue({ kind: "enabled" }); + + expect( + hasApprovalTurnSourceRoute({ + turnSourceChannel: "slack", + turnSourceAccountId: "work", + }), + ).toBe(true); + expect(resolveExecApprovalInitiatingSurfaceStateMock).toHaveBeenCalledWith({ + channel: "slack", + accountId: "work", + cfg: { loaded: true }, + }); }); - it("rejects missing or unknown turn sources", () => { - expect(hasApprovalTurnSourceRoute({ turnSourceChannel: undefined })).toBe(false); + it("returns false when the initiating surface is disabled or unsupported", () => { + resolveExecApprovalInitiatingSurfaceStateMock.mockReturnValueOnce({ kind: "disabled" }); + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "discord" })).toBe(false); + + resolveExecApprovalInitiatingSurfaceStateMock.mockReturnValueOnce({ kind: "unsupported" }); expect(hasApprovalTurnSourceRoute({ turnSourceChannel: "unknown-channel" })).toBe(false); }); + + it("returns false when there is no turn-source channel", () => { + expect(hasApprovalTurnSourceRoute({ turnSourceChannel: undefined })).toBe(false); + expect(resolveExecApprovalInitiatingSurfaceStateMock).not.toHaveBeenCalled(); + }); }); diff --git a/src/infra/approval-turn-source.ts b/src/infra/approval-turn-source.ts index a86016c4075..284ce6fe128 100644 --- a/src/infra/approval-turn-source.ts +++ b/src/infra/approval-turn-source.ts @@ -1,16 +1,18 @@ -import { - INTERNAL_MESSAGE_CHANNEL, - isDeliverableMessageChannel, - normalizeMessageChannel, -} from "../utils/message-channel.js"; +import { loadConfig } from "../config/config.js"; +import { resolveExecApprovalInitiatingSurfaceState } from "./exec-approval-surface.js"; -export function hasApprovalTurnSourceRoute(params: { turnSourceChannel?: string | null }): boolean { - const channel = normalizeMessageChannel(params.turnSourceChannel); - if (!channel) { +export function hasApprovalTurnSourceRoute(params: { + turnSourceChannel?: string | null; + turnSourceAccountId?: string | null; +}): boolean { + if (!params.turnSourceChannel?.trim()) { return false; } - if (channel === INTERNAL_MESSAGE_CHANNEL || channel === "tui") { - return true; - } - return isDeliverableMessageChannel(channel); + return ( + resolveExecApprovalInitiatingSurfaceState({ + channel: params.turnSourceChannel, + accountId: params.turnSourceAccountId, + cfg: loadConfig(), + }).kind === "enabled" + ); }