From 7d0a0ae3ba449dd113476620de512be9be0ebdfc Mon Sep 17 00:00:00 2001 From: Paul van Oorschot <20116814+pvoo@users.noreply.github.com> Date: Fri, 23 Jan 2026 21:01:15 +0100 Subject: [PATCH] fix(discord): autoThread ack reactions + exec approval null handling (#1511) * fix(discord): gate autoThread by thread owner * fix(discord): ack bot-owned autoThreads * fix(discord): ack mentions in open channels - Ack reactions in bot-owned autoThreads - Ack reactions in open channels (no mention required) - DRY: Pass pre-computed isAutoThreadOwnedByBot to avoid redundant checks - Consolidate ack logic with explanatory comment * fix: allow null values in exec.approval.request schema The ExecApprovalRequestParamsSchema was rejecting null values for optional fields like resolvedPath, but the calling code in bash-tools.exec.ts passes null. This caused intermittent 'invalid exec.approval.request params' validation errors. Fix: Accept Type.Union([Type.String(), Type.Null()]) for all optional string fields in the schema. Update test to reflect new behavior. * fix: align discord ack reactions with mention gating (#1511) (thanks @pvoo) --------- Co-authored-by: Wimmie Co-authored-by: Peter Steinberger --- CHANGELOG.md | 2 + src/discord/monitor.test.ts | 51 ++++++++ src/discord/monitor/allow-list.ts | 21 ++- .../monitor/message-handler.preflight.ts | 3 + .../monitor/message-handler.process.test.ts | 123 ++++++++++++++++++ src/discord/monitor/message-utils.ts | 3 + src/discord/monitor/threading.ts | 2 + src/gateway/protocol/schema/exec-approvals.ts | 14 +- .../server-methods/exec-approval.test.ts | 8 +- 9 files changed, 215 insertions(+), 12 deletions(-) create mode 100644 src/discord/monitor/message-handler.process.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 288458bd1..b33b621e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ Docs: https://docs.clawd.bot - Markdown: add per-channel table conversion (bullets for Signal/WhatsApp, code blocks elsewhere). (#1495) Thanks @odysseus0. ### Fixes +- Discord: limit autoThread mention bypass to bot-owned threads; keep ack reactions mention-gated. (#1511) Thanks @pvoo. +- Gateway: accept null optional fields in exec approval requests. (#1511) Thanks @pvoo. - TUI: forward unknown slash commands (for example, `/context`) to the Gateway. - TUI: include Gateway slash commands in autocomplete and `/help`. - CLI: skip usage lines in `clawdbot models status` when provider usage is unavailable. diff --git a/src/discord/monitor.test.ts b/src/discord/monitor.test.ts index be0c8aa65..bc85e5764 100644 --- a/src/discord/monitor.test.ts +++ b/src/discord/monitor.test.ts @@ -377,12 +377,63 @@ describe("discord mention gating", () => { resolveDiscordShouldRequireMention({ isGuildMessage: true, isThread: true, + botId: "bot123", + threadOwnerId: "bot123", channelConfig, guildInfo, }), ).toBe(false); }); + it("requires mention inside user-created threads with autoThread enabled", () => { + const guildInfo: DiscordGuildEntryResolved = { + requireMention: true, + channels: { + general: { allow: true, autoThread: true }, + }, + }; + const channelConfig = resolveDiscordChannelConfig({ + guildInfo, + channelId: "1", + channelName: "General", + channelSlug: "general", + }); + expect( + resolveDiscordShouldRequireMention({ + isGuildMessage: true, + isThread: true, + botId: "bot123", + threadOwnerId: "user456", + channelConfig, + guildInfo, + }), + ).toBe(true); + }); + + it("requires mention when thread owner is unknown", () => { + const guildInfo: DiscordGuildEntryResolved = { + requireMention: true, + channels: { + general: { allow: true, autoThread: true }, + }, + }; + const channelConfig = resolveDiscordChannelConfig({ + guildInfo, + channelId: "1", + channelName: "General", + channelSlug: "general", + }); + expect( + resolveDiscordShouldRequireMention({ + isGuildMessage: true, + isThread: true, + botId: "bot123", + channelConfig, + guildInfo, + }), + ).toBe(true); + }); + it("inherits parent channel mention rules for threads", () => { const guildInfo: DiscordGuildEntryResolved = { requireMention: true, diff --git a/src/discord/monitor/allow-list.ts b/src/discord/monitor/allow-list.ts index 7d495af66..12c2d1d39 100644 --- a/src/discord/monitor/allow-list.ts +++ b/src/discord/monitor/allow-list.ts @@ -282,14 +282,33 @@ export function resolveDiscordChannelConfigWithFallback(params: { export function resolveDiscordShouldRequireMention(params: { isGuildMessage: boolean; isThread: boolean; + botId?: string | null; + threadOwnerId?: string | null; channelConfig?: DiscordChannelConfigResolved | null; guildInfo?: DiscordGuildEntryResolved | null; + /** Pass pre-computed value to avoid redundant checks. */ + isAutoThreadOwnedByBot?: boolean; }): boolean { if (!params.isGuildMessage) return false; - if (params.isThread && params.channelConfig?.autoThread) return false; + // Only skip mention requirement in threads created by the bot (when autoThread is enabled). + const isBotThread = params.isAutoThreadOwnedByBot ?? isDiscordAutoThreadOwnedByBot(params); + if (isBotThread) return false; return params.channelConfig?.requireMention ?? params.guildInfo?.requireMention ?? true; } +export function isDiscordAutoThreadOwnedByBot(params: { + isThread: boolean; + channelConfig?: DiscordChannelConfigResolved | null; + botId?: string | null; + threadOwnerId?: string | null; +}): boolean { + if (!params.isThread) return false; + if (!params.channelConfig?.autoThread) return false; + const botId = params.botId?.trim(); + const threadOwnerId = params.threadOwnerId?.trim(); + return Boolean(botId && threadOwnerId && botId === threadOwnerId); +} + export function isDiscordGroupAllowedByPolicy(params: { groupPolicy: "open" | "disabled" | "allowlist"; guildAllowlisted: boolean; diff --git a/src/discord/monitor/message-handler.preflight.ts b/src/discord/monitor/message-handler.preflight.ts index 6df141e35..607b02cdd 100644 --- a/src/discord/monitor/message-handler.preflight.ts +++ b/src/discord/monitor/message-handler.preflight.ts @@ -328,9 +328,12 @@ export async function preflightDiscordMessage( } satisfies HistoryEntry) : undefined; + const threadOwnerId = threadChannel ? (threadChannel.ownerId ?? channelInfo?.ownerId) : undefined; const shouldRequireMention = resolveDiscordShouldRequireMention({ isGuildMessage, isThread: Boolean(threadChannel), + botId, + threadOwnerId, channelConfig, guildInfo, }); diff --git a/src/discord/monitor/message-handler.process.test.ts b/src/discord/monitor/message-handler.process.test.ts new file mode 100644 index 000000000..351f46f74 --- /dev/null +++ b/src/discord/monitor/message-handler.process.test.ts @@ -0,0 +1,123 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const reactMessageDiscord = vi.fn(async () => {}); +const removeReactionDiscord = vi.fn(async () => {}); + +vi.mock("../send.js", () => ({ + reactMessageDiscord: (...args: unknown[]) => reactMessageDiscord(...args), + removeReactionDiscord: (...args: unknown[]) => removeReactionDiscord(...args), +})); + +vi.mock("../../auto-reply/reply/dispatch-from-config.js", () => ({ + dispatchReplyFromConfig: vi.fn(async () => ({ + queuedFinal: false, + counts: { final: 0, tool: 0, block: 0 }, + })), +})); + +vi.mock("../../auto-reply/reply/reply-dispatcher.js", () => ({ + createReplyDispatcherWithTyping: vi.fn(() => ({ + dispatcher: {}, + replyOptions: {}, + markDispatchIdle: vi.fn(), + })), +})); + +import { processDiscordMessage } from "./message-handler.process.js"; + +async function createBaseContext(overrides: Record = {}) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "clawdbot-discord-")); + const storePath = path.join(dir, "sessions.json"); + return { + cfg: { messages: { ackReaction: "👀" }, session: { store: storePath } }, + discordConfig: {}, + accountId: "default", + token: "token", + runtime: { log: () => {}, error: () => {} }, + guildHistories: new Map(), + historyLimit: 0, + mediaMaxBytes: 1024, + textLimit: 4000, + replyToMode: "off", + ackReactionScope: "group-mentions", + groupPolicy: "open", + data: { guild: { id: "g1", name: "Guild" } }, + client: { rest: {} }, + message: { + id: "m1", + channelId: "c1", + timestamp: new Date().toISOString(), + attachments: [], + }, + author: { + id: "U1", + username: "alice", + discriminator: "0", + globalName: "Alice", + }, + channelInfo: { name: "general" }, + channelName: "general", + isGuildMessage: true, + isDirectMessage: false, + isGroupDm: false, + commandAuthorized: true, + baseText: "hi", + messageText: "hi", + wasMentioned: false, + shouldRequireMention: true, + canDetectMention: true, + effectiveWasMentioned: true, + shouldBypassMention: false, + threadChannel: null, + threadParentId: undefined, + threadParentName: undefined, + threadParentType: undefined, + threadName: undefined, + displayChannelSlug: "general", + guildInfo: null, + guildSlug: "guild", + channelConfig: null, + baseSessionKey: "agent:main:discord:guild:g1", + route: { + agentId: "main", + channel: "discord", + accountId: "default", + sessionKey: "agent:main:discord:guild:g1", + mainSessionKey: "agent:main:main", + }, + ...overrides, + }; +} + +beforeEach(() => { + reactMessageDiscord.mockClear(); + removeReactionDiscord.mockClear(); +}); + +describe("processDiscordMessage ack reactions", () => { + it("skips ack reactions for group-mentions when mentions are not required", async () => { + const ctx = await createBaseContext({ + shouldRequireMention: false, + effectiveWasMentioned: false, + }); + + await processDiscordMessage(ctx as any); + + expect(reactMessageDiscord).not.toHaveBeenCalled(); + }); + + it("sends ack reactions for mention-gated guild messages when mentioned", async () => { + const ctx = await createBaseContext({ + shouldRequireMention: true, + effectiveWasMentioned: true, + }); + + await processDiscordMessage(ctx as any); + + expect(reactMessageDiscord).toHaveBeenCalledWith("c1", "m1", "👀", { rest: {} }); + }); +}); diff --git a/src/discord/monitor/message-utils.ts b/src/discord/monitor/message-utils.ts index a681afa16..2647e5113 100644 --- a/src/discord/monitor/message-utils.ts +++ b/src/discord/monitor/message-utils.ts @@ -16,6 +16,7 @@ export type DiscordChannelInfo = { name?: string; topic?: string; parentId?: string; + ownerId?: string; }; type DiscordSnapshotAuthor = { @@ -69,11 +70,13 @@ export async function resolveDiscordChannelInfo( const name = "name" in channel ? (channel.name ?? undefined) : undefined; const topic = "topic" in channel ? (channel.topic ?? undefined) : undefined; const parentId = "parentId" in channel ? (channel.parentId ?? undefined) : undefined; + const ownerId = "ownerId" in channel ? (channel.ownerId ?? undefined) : undefined; const payload: DiscordChannelInfo = { type: channel.type, name, topic, parentId, + ownerId, }; DISCORD_CHANNEL_INFO_CACHE.set(channelId, { value: payload, diff --git a/src/discord/monitor/threading.ts b/src/discord/monitor/threading.ts index bae4ef1c5..71af6408f 100644 --- a/src/discord/monitor/threading.ts +++ b/src/discord/monitor/threading.ts @@ -14,6 +14,7 @@ export type DiscordThreadChannel = { name?: string | null; parentId?: string | null; parent?: { id?: string; name?: string }; + ownerId?: string | null; }; export type DiscordThreadStarter = { @@ -63,6 +64,7 @@ export function resolveDiscordThreadChannel(params: { name: channelInfo?.name ?? undefined, parentId: channelInfo?.parentId ?? undefined, parent: undefined, + ownerId: channelInfo?.ownerId ?? undefined, }; } diff --git a/src/gateway/protocol/schema/exec-approvals.ts b/src/gateway/protocol/schema/exec-approvals.ts index d58e74ab2..e6f7ce906 100644 --- a/src/gateway/protocol/schema/exec-approvals.ts +++ b/src/gateway/protocol/schema/exec-approvals.ts @@ -92,13 +92,13 @@ export const ExecApprovalRequestParamsSchema = Type.Object( { id: Type.Optional(NonEmptyString), command: NonEmptyString, - cwd: Type.Optional(Type.String()), - host: Type.Optional(Type.String()), - security: Type.Optional(Type.String()), - ask: Type.Optional(Type.String()), - agentId: Type.Optional(Type.String()), - resolvedPath: Type.Optional(Type.String()), - sessionKey: Type.Optional(Type.String()), + cwd: Type.Optional(Type.Union([Type.String(), Type.Null()])), + host: Type.Optional(Type.Union([Type.String(), Type.Null()])), + security: Type.Optional(Type.Union([Type.String(), Type.Null()])), + ask: Type.Optional(Type.Union([Type.String(), Type.Null()])), + agentId: Type.Optional(Type.Union([Type.String(), Type.Null()])), + resolvedPath: Type.Optional(Type.Union([Type.String(), Type.Null()])), + sessionKey: Type.Optional(Type.Union([Type.String(), Type.Null()])), timeoutMs: Type.Optional(Type.Integer({ minimum: 1 })), }, { additionalProperties: false }, diff --git a/src/gateway/server-methods/exec-approval.test.ts b/src/gateway/server-methods/exec-approval.test.ts index 0b1da93f3..71a63e5a3 100644 --- a/src/gateway/server-methods/exec-approval.test.ts +++ b/src/gateway/server-methods/exec-approval.test.ts @@ -36,16 +36,16 @@ describe("exec approval handlers", () => { expect(validateExecApprovalRequestParams(params)).toBe(true); }); - // This documents the TypeBox/AJV behavior that caused the Discord exec bug: - // Type.Optional(Type.String()) does NOT accept null, only string or undefined. - it("rejects request with resolvedPath as null", () => { + // Fixed: null is now accepted (Type.Union([Type.String(), Type.Null()])) + // This matches the calling code in bash-tools.exec.ts which passes null. + it("accepts request with resolvedPath as null", () => { const params = { command: "echo hi", cwd: "/tmp", host: "node", resolvedPath: null, }; - expect(validateExecApprovalRequestParams(params)).toBe(false); + expect(validateExecApprovalRequestParams(params)).toBe(true); }); });