From 548c280eff60d1851473fdb455cfdc82dd552779 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 29 Apr 2026 06:29:36 +0100 Subject: [PATCH] fix(discord): keep exec approval fallbacks reachable --- CHANGELOG.md | 1 + docs/channels/discord.md | 5 ++ docs/tools/exec-approvals-advanced.md | 6 ++ extensions/discord/src/channel.ts | 3 +- extensions/discord/src/exec-approvals.ts | 17 ++++- src/agents/system-prompt.test.ts | 17 +++++ src/agents/system-prompt.ts | 6 +- .../reply/dispatch-from-config.test.ts | 73 ++++++++++++++++++- src/channels/plugins/exec-approval-local.ts | 11 ++- src/channels/plugins/outbound.types.ts | 6 +- .../approval-native-route-coordinator.test.ts | 62 ++++++++++++++++ .../approval-native-route-coordinator.ts | 51 +++++++++++++ src/infra/approval-native-route-notice.ts | 21 ++++++ src/plugin-sdk/channel-contract.ts | 1 + 14 files changed, 270 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fae4f6c2705..27b3f9e2f8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,7 @@ Docs: https://docs.openclaw.ai - OpenAI-compatible providers: drop malformed event-only or blank-data SSE frames before the OpenAI SDK stream parser sees them, so proxies that split `event:` from `data:` no longer crash streaming runs with `Unexpected end of JSON input`. Fixes #52802. Thanks @LyHug. - Gateway/OpenAI-compatible streaming: strip `` tags split across streamed model deltas before they reach SSE clients, so `/v1/chat/completions` no longer emits tag remnants or drops content when final-answer wrappers cross chunk boundaries. Fixes #63325. Thanks @tzwickl. - Ollama: resolve explicitly selected signed-in `:cloud` models through `/api/show` when `/api/tags` omits them, so working models such as `gemini-3-flash-preview:cloud` and `deepseek-v4-pro:cloud` do not fail dynamic model resolution before the native `/api/chat` transport runs. Fixes #73909. Thanks @chtse53. +- Discord/exec approvals: keep the local `/approve` prompt when no native Discord approval runtime is active, and send a manual fallback notice when native approval delivery reaches no targets, so failed DM cards no longer leave approval turns silent or dependent on model-written shell commands. Fixes #73954; carries forward #74027. Thanks @guarismo and @brokemac79. - Local model prompt caching: keep stable Project Context above volatile channel/session prompt guidance and stop embedding current channel names in the message tool description, so Ollama, MLX, llama.cpp, and other prefix-cache backends avoid avoidable full prompt reprocessing across channel turns. Fixes #40256; supersedes #40296. Thanks @rhclaw and @sriram369. - Gateway/OpenAI-compatible API: guard provider policy lookup against runtime providers with non-array `models` values, so `/v1/chat/completions` no longer fails with `provider?.models?.some is not a function`. Fixes #66744; carries forward #66761. Thanks @MightyMoud, @MukundaKatta. - WhatsApp/Web: pass explicit Baileys socket timings into every WhatsApp Web socket and expose `web.whatsapp.*` keepalive, connect, and query timeout settings so unstable networks can avoid repeated 408 disconnect and opening-handshake timeout loops. Fixes #56365. (#73580) Thanks @velvet-shark. diff --git a/docs/channels/discord.md b/docs/channels/discord.md index 6f4181ce1a4..8bc846bcfea 100644 --- a/docs/channels/discord.md +++ b/docs/channels/discord.md @@ -912,6 +912,11 @@ Default slash command settings: When those buttons are present, they are the primary approval UX; OpenClaw should only include a manual `/approve` command when the tool result says chat approvals are unavailable or manual approval is the only path. + If the Discord native approval runtime is not active, OpenClaw keeps the + local deterministic `/approve ` prompt visible. If the + runtime is active but a native card cannot be delivered to any target, + OpenClaw sends a same-chat fallback notice with the exact `/approve` + command from the pending approval. Gateway auth and approval resolution follow the shared Gateway client contract (`plugin:` IDs resolve through `plugin.approval.resolve`; other IDs through `exec.approval.resolve`). Approvals expire after 30 minutes by default. diff --git a/docs/tools/exec-approvals-advanced.md b/docs/tools/exec-approvals-advanced.md index a04050c2543..4ede6fdf3d0 100644 --- a/docs/tools/exec-approvals-advanced.md +++ b/docs/tools/exec-approvals-advanced.md @@ -262,6 +262,12 @@ agent-facing path. The agent should not also echo a duplicate plain chat `/approve` command unless the tool result says chat approvals are unavailable or manual approval is the only remaining path. +If a native approval client is configured but no native runtime is active for +the originating channel, OpenClaw keeps the local deterministic `/approve` +prompt visible. If the native runtime is active and attempts delivery but no +target receives the card, OpenClaw sends a same-chat fallback notice with the +exact `/approve ` command so the request can still be resolved. + Generic model: - host exec policy still decides whether exec approval is required diff --git a/extensions/discord/src/channel.ts b/extensions/discord/src/channel.ts index 365b5552afa..729d31af0e2 100644 --- a/extensions/discord/src/channel.ts +++ b/extensions/discord/src/channel.ts @@ -791,11 +791,12 @@ export const discordPlugin: ChannelPlugin ...discordOutbound, preferFinalAssistantVisibleText: true, shouldTreatDeliveredTextAsVisible: shouldTreatDiscordDeliveredTextAsVisible, - shouldSuppressLocalPayloadPrompt: ({ cfg, accountId, payload }) => + shouldSuppressLocalPayloadPrompt: ({ cfg, accountId, payload, hint }) => shouldSuppressLocalDiscordExecApprovalPrompt({ cfg, accountId, payload, + hint, }), }, }); diff --git a/extensions/discord/src/exec-approvals.ts b/extensions/discord/src/exec-approvals.ts index ad37b50ecdb..de2f415dba5 100644 --- a/extensions/discord/src/exec-approvals.ts +++ b/extensions/discord/src/exec-approvals.ts @@ -1,3 +1,4 @@ +import type { ChannelOutboundPayloadHint } from "openclaw/plugin-sdk/channel-contract"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-types"; import type { DiscordExecApprovalConfig } from "openclaw/plugin-sdk/config-types"; import type { ReplyPayload } from "openclaw/plugin-sdk/reply-dispatch-runtime"; @@ -5,6 +6,7 @@ import { resolveDiscordAccount } from "./accounts.js"; import { getExecApprovalReplyMetadata, isChannelExecApprovalClientEnabledFromConfig, + matchesApprovalRequestFilters, resolveApprovalApprovers, } from "./approval-runtime.js"; import { parseDiscordTarget } from "./target-parsing.js"; @@ -87,9 +89,22 @@ export function shouldSuppressLocalDiscordExecApprovalPrompt(params: { cfg: OpenClawConfig; accountId?: string | null; payload: ReplyPayload; + hint?: ChannelOutboundPayloadHint; }): boolean { + const metadata = getExecApprovalReplyMetadata(params.payload); + const config = resolveDiscordAccount(params).config.execApprovals; return ( + params.hint?.kind === "approval-pending" && + params.hint.nativeRouteActive === true && isDiscordExecApprovalClientEnabled(params) && - getExecApprovalReplyMetadata(params.payload) !== null + metadata !== null && + matchesApprovalRequestFilters({ + request: { + agentId: metadata.agentId, + sessionKey: metadata.sessionKey, + }, + agentFilter: config?.agentFilter, + sessionFilter: config?.sessionFilter, + }) ); } diff --git a/src/agents/system-prompt.test.ts b/src/agents/system-prompt.test.ts index 788a1add1d1..9b4e978279e 100644 --- a/src/agents/system-prompt.test.ts +++ b/src/agents/system-prompt.test.ts @@ -792,6 +792,23 @@ describe("buildAgentSystemPrompt", () => { expect(prompt).toContain("do not also send plain chat /approve instructions"); }); + it("keeps approval slug guidance separate from command previews", () => { + const prompt = buildAgentSystemPrompt({ + workspaceDir: "/tmp/openclaw", + runtimeInfo: { + channel: "discord", + }, + }); + + expect(prompt).toContain( + 'copy the exact /approve command from the tool output\'s "Reply with:" line', + ); + expect(prompt).toContain("keep command/script previews separate from the /approve command"); + expect(prompt).toContain( + "never substitute the shell command/script for the approval id or slug", + ); + }); + it("includes runtime provider capabilities when present", () => { const prompt = buildAgentSystemPrompt({ workspaceDir: "/tmp/openclaw", diff --git a/src/agents/system-prompt.ts b/src/agents/system-prompt.ts index 0ecc5143139..d393ec340e9 100644 --- a/src/agents/system-prompt.ts +++ b/src/agents/system-prompt.ts @@ -149,9 +149,9 @@ function buildExecApprovalPromptGuidance(params: { ? Boolean(resolveChannelApprovalCapability(getChannelPlugin(runtimeChannel))?.native) : false); if (usesNativeApprovalUi) { - return "When exec returns approval-pending on this channel, rely on native approval card/buttons when they appear and do not also send plain chat /approve instructions. Only include the concrete /approve command if the tool result says chat approvals are unavailable or only manual approval is possible."; + return 'When exec returns approval-pending on this channel, rely on native approval card/buttons when they appear and do not also send plain chat /approve instructions. Only include the concrete /approve command if the tool result says chat approvals are unavailable or only manual approval is possible; when needed, copy the exact /approve command from the tool output\'s "Reply with:" line.'; } - return "When exec returns approval-pending, include the concrete /approve command from tool output as plain chat text for the user, and do not ask for a different or rotated code."; + return 'When exec returns approval-pending, include the concrete /approve command from the tool output\'s "Reply with:" line as plain chat text for the user, and do not ask for a different or rotated code.'; } function buildSkillsSection(params: { skillsPrompt?: string; readToolName: string }) { @@ -769,7 +769,7 @@ export function buildAgentSystemPrompt(params: { }), "Never execute /approve through exec or any other shell/tool path; /approve is a user-facing approval command, not a shell command.", "Treat allow-once as single-command only: if another elevated command needs approval, request a fresh /approve and do not claim prior approval covered it.", - "When approvals are required, preserve and show the full command/script exactly as provided (including chained operators like &&, ||, |, ;, or multiline shells) so the user can approve what will actually run.", + "When approvals are required, preserve and show the full command/script exactly as provided (including chained operators like &&, ||, |, ;, or multiline shells) so the user can approve what will actually run, but keep command/script previews separate from the /approve command and never substitute the shell command/script for the approval id or slug.", "", ], }), diff --git a/src/auto-reply/reply/dispatch-from-config.test.ts b/src/auto-reply/reply/dispatch-from-config.test.ts index 665cb2f14fd..9dcf72080ee 100644 --- a/src/auto-reply/reply/dispatch-from-config.test.ts +++ b/src/auto-reply/reply/dispatch-from-config.test.ts @@ -1,5 +1,9 @@ import { beforeAll, beforeEach, describe, expect, it, vi, type Mock } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; +import { + clearApprovalNativeRouteStateForTest, + createApprovalNativeRouteReporter, +} from "../../infra/approval-native-route-coordinator.js"; import type { SessionBindingRecord } from "../../infra/outbound/session-binding-service.js"; import type { AcpRuntime, @@ -701,7 +705,14 @@ describe("dispatchReplyFromConfig", () => { }), outbound: { deliveryMode: "direct", - shouldSuppressLocalPayloadPrompt: ({ payload }: { payload: ReplyPayload }) => + shouldSuppressLocalPayloadPrompt: ({ + payload, + hint, + }: { + payload: ReplyPayload; + hint?: { nativeRouteActive?: boolean }; + }) => + hint?.nativeRouteActive === true && Boolean( payload.channelData && typeof payload.channelData === "object" && @@ -719,6 +730,7 @@ describe("dispatchReplyFromConfig", () => { }, ]), ); + clearApprovalNativeRouteStateForTest(); acpManagerRuntimeMocks.getAcpSessionManager.mockReset(); acpManagerRuntimeMocks.getAcpSessionManager.mockReturnValue(createMockAcpSessionManager()); resetInboundDedupe(); @@ -2581,7 +2593,7 @@ describe("dispatchReplyFromConfig", () => { expect(replyResolver).toHaveBeenCalledTimes(1); }); - it("suppresses local discord exec approval tool prompts when discord approvals are enabled", async () => { + it("keeps local discord exec approval tool prompts when the native runtime is inactive", async () => { setNoAbort(); const cfg = { channels: { @@ -2616,12 +2628,67 @@ describe("dispatchReplyFromConfig", () => { await dispatchReplyFromConfig({ ctx, cfg, dispatcher, replyResolver }); - expect(dispatcher.sendToolResult).not.toHaveBeenCalled(); + expect(dispatcher.sendToolResult).toHaveBeenCalledWith( + expect.objectContaining({ text: "Approval required." }), + ); expect(dispatcher.sendFinalReply).toHaveBeenCalledWith( expect.objectContaining({ text: "done" }), ); }); + it("suppresses local discord exec approval tool prompts when the native runtime is active", async () => { + setNoAbort(); + const cfg = { + channels: { + discord: { + enabled: true, + execApprovals: { + enabled: true, + approvers: ["123"], + }, + }, + }, + } as OpenClawConfig; + const reporter = createApprovalNativeRouteReporter({ + handledKinds: new Set(["exec"]), + channel: "discord", + channelLabel: "Discord", + accountId: "default", + requestGateway: async () => ({ ok: true }) as T, + }); + reporter.start(); + try { + const dispatcher = createDispatcher(); + const ctx = buildTestCtx({ + Provider: "discord", + Surface: "discord", + AccountId: "default", + }); + const replyResolver = vi.fn(async (_ctx: MsgContext, options?: GetReplyOptions) => { + await options?.onToolResult?.({ + text: "Approval required.", + channelData: { + execApproval: { + approvalId: "12345678-1234-1234-1234-123456789012", + approvalSlug: "12345678", + allowedDecisions: ["allow-once", "allow-always", "deny"], + }, + }, + }); + return { text: "done" } as ReplyPayload; + }); + + await dispatchReplyFromConfig({ ctx, cfg, dispatcher, replyResolver }); + + expect(dispatcher.sendToolResult).not.toHaveBeenCalled(); + expect(dispatcher.sendFinalReply).toHaveBeenCalledWith( + expect.objectContaining({ text: "done" }), + ); + } finally { + await reporter.stop(); + } + }); + it("deduplicates same-agent inbound replies across main and direct session keys", async () => { setNoAbort(); const cfg = emptyConfig; diff --git a/src/channels/plugins/exec-approval-local.ts b/src/channels/plugins/exec-approval-local.ts index 5c01d753a09..912392a4394 100644 --- a/src/channels/plugins/exec-approval-local.ts +++ b/src/channels/plugins/exec-approval-local.ts @@ -1,5 +1,6 @@ import type { ReplyPayload } from "../../auto-reply/reply-payload.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; +import { hasActiveApprovalNativeRouteRuntime } from "../../infra/approval-native-route-coordinator.js"; import { getChannelPlugin, normalizeChannelId } from "./registry.js"; export function shouldSuppressLocalExecApprovalPrompt(params: { @@ -17,7 +18,15 @@ export function shouldSuppressLocalExecApprovalPrompt(params: { cfg: params.cfg, accountId: params.accountId, payload: params.payload, - hint: { kind: "approval-pending", approvalKind: "exec" }, + hint: { + kind: "approval-pending", + approvalKind: "exec", + nativeRouteActive: hasActiveApprovalNativeRouteRuntime({ + channel, + accountId: params.accountId, + approvalKind: "exec", + }), + }, }) ?? false ); } diff --git a/src/channels/plugins/outbound.types.ts b/src/channels/plugins/outbound.types.ts index 9237f9a1cc4..71a48c637b8 100644 --- a/src/channels/plugins/outbound.types.ts +++ b/src/channels/plugins/outbound.types.ts @@ -54,7 +54,11 @@ export type ChannelDeliveryCapabilities = { }; export type ChannelOutboundPayloadHint = - | { kind: "approval-pending"; approvalKind: "exec" | "plugin" } + | { + kind: "approval-pending"; + approvalKind: "exec" | "plugin"; + nativeRouteActive?: boolean; + } | { kind: "approval-resolved"; approvalKind: "exec" | "plugin" }; export type ChannelOutboundTargetRef = { diff --git a/src/infra/approval-native-route-coordinator.test.ts b/src/infra/approval-native-route-coordinator.test.ts index ac2e8ce803e..bd9dff8db4e 100644 --- a/src/infra/approval-native-route-coordinator.test.ts +++ b/src/infra/approval-native-route-coordinator.test.ts @@ -212,4 +212,66 @@ describe("createApprovalNativeRouteReporter", () => { }); expect(otherGateway).not.toHaveBeenCalled(); }); + + it("sends a manual fallback notice when native delivery reaches no targets", async () => { + const requestGateway = createGatewayRequestMock(); + const request = { + id: "deadbeef-1234-4567-89ab-cdef01234567", + request: { + command: "echo hi", + allowedDecisions: ["allow-once", "deny"], + turnSourceChannel: "discord", + turnSourceTo: "channel:C123", + turnSourceAccountId: "default", + }, + createdAtMs: 0, + expiresAtMs: Date.now() + 60_000, + } as const; + + const reporter = createApprovalNativeRouteReporter({ + handledKinds: new Set(["exec"]), + channel: "discord", + channelLabel: "Discord", + accountId: "default", + requestGateway, + }); + reporter.start(); + reporter.observeRequest({ + approvalKind: "exec", + request, + }); + + await reporter.reportDelivery({ + approvalKind: "exec", + request, + deliveryPlan: { + targets: [ + { + surface: "approver-dm", + target: { + to: "user:owner", + }, + reason: "preferred", + }, + ], + originTarget: { + to: "channel:C123", + }, + notifyOriginWhenDmOnly: true, + }, + deliveredTargets: [], + }); + + expect(requestGateway).toHaveBeenCalledWith("send", { + channel: "discord", + to: "channel:C123", + accountId: "default", + threadId: undefined, + message: + "Approval required. I could not deliver the native approval request.\n" + + "Reply with: /approve deadbeef allow-once|deny\n" + + "If the short code is ambiguous, use the full id in /approve.", + idempotencyKey: "approval-route-notice:deadbeef-1234-4567-89ab-cdef01234567", + }); + }); }); diff --git a/src/infra/approval-native-route-coordinator.ts b/src/infra/approval-native-route-coordinator.ts index a2fbeb45cae..b0b65e712bf 100644 --- a/src/infra/approval-native-route-coordinator.ts +++ b/src/infra/approval-native-route-coordinator.ts @@ -8,6 +8,7 @@ import type { } from "./approval-native-delivery.js"; import { describeApprovalDeliveryDestination, + resolveApprovalDeliveryFailedNoticeText, resolveApprovalRoutedElsewhereNoticeText, } from "./approval-native-route-notice.js"; import { buildChannelApprovalNativeTargetKey } from "./approval-native-target-key.js"; @@ -151,7 +152,21 @@ function didReportDeliverToOrigin(report: ApprovalRouteReport, originAccountId?: ); } +function hasPlannedNativeTargets(report: ApprovalRouteReport): boolean { + return report.deliveryPlan.targets.length > 0; +} + +function readAllowedDecisionStrings(request: ApprovalRequest): string[] | undefined { + const allowedDecisions = + "allowedDecisions" in request.request ? request.request.allowedDecisions : undefined; + if (!Array.isArray(allowedDecisions)) { + return undefined; + } + return allowedDecisions.filter((value): value is string => typeof value === "string"); +} + function resolveApprovalRouteNotice(params: { + approvalKind: ChannelApprovalKind; request: ApprovalRequest; reports: readonly ApprovalRouteReport[]; }): { requestGateway: GatewayRequestFn; target: RouteNoticeTarget; text: string } | null { @@ -176,6 +191,20 @@ function resolveApprovalRouteNotice(params: { return null; } const originAccountId = normalizeOptionalString(target.accountId); + const deliveredAnyTarget = params.reports.some((report) => report.deliveredTargets.length > 0); + if (!deliveredAnyTarget && params.reports.some(hasPlannedNativeTargets)) { + return { + requestGateway: + params.reports.find((report) => activeApprovalRouteRuntimes.has(report.runtimeId)) + ?.requestGateway ?? params.reports[0].requestGateway, + target, + text: resolveApprovalDeliveryFailedNoticeText({ + approvalId: params.request.id, + approvalKind: params.approvalKind, + allowedDecisions: readAllowedDecisionStrings(params.request), + }), + }; + } // If any same-channel runtime already delivered into the origin chat, every // other fallback delivery becomes supplemental and should not trigger a notice. @@ -237,6 +266,27 @@ function resolveApprovalRouteNotice(params: { }; } +export function hasActiveApprovalNativeRouteRuntime(params: { + approvalKind: ChannelApprovalKind; + channel?: string | null; + accountId?: string | null; +}): boolean { + const channel = normalizeChannel(params.channel); + const accountId = normalizeOptionalString(params.accountId); + return Array.from(activeApprovalRouteRuntimes.values()).some((runtime) => { + if (!runtime.handledKinds.has(params.approvalKind)) { + return false; + } + if (channel && normalizeChannel(runtime.channel) !== channel) { + return false; + } + const runtimeAccountId = normalizeOptionalString(runtime.accountId); + return ( + accountId === undefined || runtimeAccountId === undefined || runtimeAccountId === accountId + ); + }); +} + async function maybeFinalizeApprovalRouteNotice(approvalId: string): Promise { const entry = pendingApprovalRouteNotices.get(approvalId); if (!entry || entry.finalized) { @@ -251,6 +301,7 @@ async function maybeFinalizeApprovalRouteNotice(approvalId: string): Promise a.localeCompare(b)), )}, not this chat.`; } + +export function resolveApprovalDeliveryFailedNoticeText(params: { + approvalId: string; + approvalKind: "exec" | "plugin"; + allowedDecisions?: readonly string[]; +}): string { + const commandId = + params.approvalKind === "exec" && params.approvalId.length > 8 + ? params.approvalId.slice(0, 8) + : params.approvalId; + const decisions = ( + params.allowedDecisions?.length + ? params.allowedDecisions + : ["allow-once", "allow-always", "deny"] + ).join("|"); + return [ + "Approval required. I could not deliver the native approval request.", + `Reply with: /approve ${commandId} ${decisions}`, + "If the short code is ambiguous, use the full id in /approve.", + ].join("\n"); +} diff --git a/src/plugin-sdk/channel-contract.ts b/src/plugin-sdk/channel-contract.ts index 3c962d855eb..97824c8cec8 100644 --- a/src/plugin-sdk/channel-contract.ts +++ b/src/plugin-sdk/channel-contract.ts @@ -38,6 +38,7 @@ export type { ChannelGatewayContext, ChannelOutboundAdapter, ChannelOutboundContext, + ChannelOutboundPayloadHint, ChannelStatusAdapter, } from "../channels/plugins/types.adapters.js"; export type { ChannelRuntimeSurface } from "../channels/plugins/channel-runtime-surface.types.js";