mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-31 14:26:30 +02:00
fix: make same-chat approvals work across channels
This commit is contained in:
@@ -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`
|
||||
|
||||
@@ -492,10 +492,7 @@ export const discordPlugin: ChannelPlugin<ResolvedDiscordAccount, DiscordProbe>
|
||||
},
|
||||
},
|
||||
execApprovals: {
|
||||
getInitiatingSurfaceState: ({ cfg, accountId }) =>
|
||||
isDiscordExecApprovalClientEnabled({ cfg, accountId })
|
||||
? { kind: "enabled" }
|
||||
: { kind: "disabled" },
|
||||
getInitiatingSurfaceState: () => ({ kind: "enabled" }),
|
||||
shouldSuppressLocalPrompt: ({ cfg, accountId, payload }) =>
|
||||
shouldSuppressLocalDiscordExecApprovalPrompt({
|
||||
cfg,
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, unknown>)?.status === "accepted",
|
||||
);
|
||||
const approvalId = (acceptedCall?.[1] as Record<string, unknown>)?.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", {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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();
|
||||
|
||||
19
src/infra/approval-turn-source.test.ts
Normal file
19
src/infra/approval-turn-source.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
});
|
||||
16
src/infra/approval-turn-source.ts
Normal file
16
src/infra/approval-turn-source.ts
Normal file
@@ -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);
|
||||
}
|
||||
@@ -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<typeof import("../config/config.js")>();
|
||||
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", () => {
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user