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 <wimmie@tameson.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
committed by
GitHub
parent
242add587f
commit
7d0a0ae3ba
@@ -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.
|
- Markdown: add per-channel table conversion (bullets for Signal/WhatsApp, code blocks elsewhere). (#1495) Thanks @odysseus0.
|
||||||
|
|
||||||
### Fixes
|
### 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: forward unknown slash commands (for example, `/context`) to the Gateway.
|
||||||
- TUI: include Gateway slash commands in autocomplete and `/help`.
|
- TUI: include Gateway slash commands in autocomplete and `/help`.
|
||||||
- CLI: skip usage lines in `clawdbot models status` when provider usage is unavailable.
|
- CLI: skip usage lines in `clawdbot models status` when provider usage is unavailable.
|
||||||
|
|||||||
@@ -377,12 +377,63 @@ describe("discord mention gating", () => {
|
|||||||
resolveDiscordShouldRequireMention({
|
resolveDiscordShouldRequireMention({
|
||||||
isGuildMessage: true,
|
isGuildMessage: true,
|
||||||
isThread: true,
|
isThread: true,
|
||||||
|
botId: "bot123",
|
||||||
|
threadOwnerId: "bot123",
|
||||||
channelConfig,
|
channelConfig,
|
||||||
guildInfo,
|
guildInfo,
|
||||||
}),
|
}),
|
||||||
).toBe(false);
|
).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", () => {
|
it("inherits parent channel mention rules for threads", () => {
|
||||||
const guildInfo: DiscordGuildEntryResolved = {
|
const guildInfo: DiscordGuildEntryResolved = {
|
||||||
requireMention: true,
|
requireMention: true,
|
||||||
|
|||||||
@@ -282,14 +282,33 @@ export function resolveDiscordChannelConfigWithFallback(params: {
|
|||||||
export function resolveDiscordShouldRequireMention(params: {
|
export function resolveDiscordShouldRequireMention(params: {
|
||||||
isGuildMessage: boolean;
|
isGuildMessage: boolean;
|
||||||
isThread: boolean;
|
isThread: boolean;
|
||||||
|
botId?: string | null;
|
||||||
|
threadOwnerId?: string | null;
|
||||||
channelConfig?: DiscordChannelConfigResolved | null;
|
channelConfig?: DiscordChannelConfigResolved | null;
|
||||||
guildInfo?: DiscordGuildEntryResolved | null;
|
guildInfo?: DiscordGuildEntryResolved | null;
|
||||||
|
/** Pass pre-computed value to avoid redundant checks. */
|
||||||
|
isAutoThreadOwnedByBot?: boolean;
|
||||||
}): boolean {
|
}): boolean {
|
||||||
if (!params.isGuildMessage) return false;
|
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;
|
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: {
|
export function isDiscordGroupAllowedByPolicy(params: {
|
||||||
groupPolicy: "open" | "disabled" | "allowlist";
|
groupPolicy: "open" | "disabled" | "allowlist";
|
||||||
guildAllowlisted: boolean;
|
guildAllowlisted: boolean;
|
||||||
|
|||||||
@@ -328,9 +328,12 @@ export async function preflightDiscordMessage(
|
|||||||
} satisfies HistoryEntry)
|
} satisfies HistoryEntry)
|
||||||
: undefined;
|
: undefined;
|
||||||
|
|
||||||
|
const threadOwnerId = threadChannel ? (threadChannel.ownerId ?? channelInfo?.ownerId) : undefined;
|
||||||
const shouldRequireMention = resolveDiscordShouldRequireMention({
|
const shouldRequireMention = resolveDiscordShouldRequireMention({
|
||||||
isGuildMessage,
|
isGuildMessage,
|
||||||
isThread: Boolean(threadChannel),
|
isThread: Boolean(threadChannel),
|
||||||
|
botId,
|
||||||
|
threadOwnerId,
|
||||||
channelConfig,
|
channelConfig,
|
||||||
guildInfo,
|
guildInfo,
|
||||||
});
|
});
|
||||||
|
|||||||
123
src/discord/monitor/message-handler.process.test.ts
Normal file
123
src/discord/monitor/message-handler.process.test.ts
Normal file
@@ -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<string, unknown> = {}) {
|
||||||
|
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: {} });
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -16,6 +16,7 @@ export type DiscordChannelInfo = {
|
|||||||
name?: string;
|
name?: string;
|
||||||
topic?: string;
|
topic?: string;
|
||||||
parentId?: string;
|
parentId?: string;
|
||||||
|
ownerId?: string;
|
||||||
};
|
};
|
||||||
|
|
||||||
type DiscordSnapshotAuthor = {
|
type DiscordSnapshotAuthor = {
|
||||||
@@ -69,11 +70,13 @@ export async function resolveDiscordChannelInfo(
|
|||||||
const name = "name" in channel ? (channel.name ?? undefined) : undefined;
|
const name = "name" in channel ? (channel.name ?? undefined) : undefined;
|
||||||
const topic = "topic" in channel ? (channel.topic ?? undefined) : undefined;
|
const topic = "topic" in channel ? (channel.topic ?? undefined) : undefined;
|
||||||
const parentId = "parentId" in channel ? (channel.parentId ?? undefined) : undefined;
|
const parentId = "parentId" in channel ? (channel.parentId ?? undefined) : undefined;
|
||||||
|
const ownerId = "ownerId" in channel ? (channel.ownerId ?? undefined) : undefined;
|
||||||
const payload: DiscordChannelInfo = {
|
const payload: DiscordChannelInfo = {
|
||||||
type: channel.type,
|
type: channel.type,
|
||||||
name,
|
name,
|
||||||
topic,
|
topic,
|
||||||
parentId,
|
parentId,
|
||||||
|
ownerId,
|
||||||
};
|
};
|
||||||
DISCORD_CHANNEL_INFO_CACHE.set(channelId, {
|
DISCORD_CHANNEL_INFO_CACHE.set(channelId, {
|
||||||
value: payload,
|
value: payload,
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ export type DiscordThreadChannel = {
|
|||||||
name?: string | null;
|
name?: string | null;
|
||||||
parentId?: string | null;
|
parentId?: string | null;
|
||||||
parent?: { id?: string; name?: string };
|
parent?: { id?: string; name?: string };
|
||||||
|
ownerId?: string | null;
|
||||||
};
|
};
|
||||||
|
|
||||||
export type DiscordThreadStarter = {
|
export type DiscordThreadStarter = {
|
||||||
@@ -63,6 +64,7 @@ export function resolveDiscordThreadChannel(params: {
|
|||||||
name: channelInfo?.name ?? undefined,
|
name: channelInfo?.name ?? undefined,
|
||||||
parentId: channelInfo?.parentId ?? undefined,
|
parentId: channelInfo?.parentId ?? undefined,
|
||||||
parent: undefined,
|
parent: undefined,
|
||||||
|
ownerId: channelInfo?.ownerId ?? undefined,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -92,13 +92,13 @@ export const ExecApprovalRequestParamsSchema = Type.Object(
|
|||||||
{
|
{
|
||||||
id: Type.Optional(NonEmptyString),
|
id: Type.Optional(NonEmptyString),
|
||||||
command: NonEmptyString,
|
command: NonEmptyString,
|
||||||
cwd: Type.Optional(Type.String()),
|
cwd: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
host: Type.Optional(Type.String()),
|
host: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
security: Type.Optional(Type.String()),
|
security: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
ask: Type.Optional(Type.String()),
|
ask: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
agentId: Type.Optional(Type.String()),
|
agentId: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
resolvedPath: Type.Optional(Type.String()),
|
resolvedPath: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
sessionKey: Type.Optional(Type.String()),
|
sessionKey: Type.Optional(Type.Union([Type.String(), Type.Null()])),
|
||||||
timeoutMs: Type.Optional(Type.Integer({ minimum: 1 })),
|
timeoutMs: Type.Optional(Type.Integer({ minimum: 1 })),
|
||||||
},
|
},
|
||||||
{ additionalProperties: false },
|
{ additionalProperties: false },
|
||||||
|
|||||||
@@ -36,16 +36,16 @@ describe("exec approval handlers", () => {
|
|||||||
expect(validateExecApprovalRequestParams(params)).toBe(true);
|
expect(validateExecApprovalRequestParams(params)).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
// This documents the TypeBox/AJV behavior that caused the Discord exec bug:
|
// Fixed: null is now accepted (Type.Union([Type.String(), Type.Null()]))
|
||||||
// Type.Optional(Type.String()) does NOT accept null, only string or undefined.
|
// This matches the calling code in bash-tools.exec.ts which passes null.
|
||||||
it("rejects request with resolvedPath as null", () => {
|
it("accepts request with resolvedPath as null", () => {
|
||||||
const params = {
|
const params = {
|
||||||
command: "echo hi",
|
command: "echo hi",
|
||||||
cwd: "/tmp",
|
cwd: "/tmp",
|
||||||
host: "node",
|
host: "node",
|
||||||
resolvedPath: null,
|
resolvedPath: null,
|
||||||
};
|
};
|
||||||
expect(validateExecApprovalRequestParams(params)).toBe(false);
|
expect(validateExecApprovalRequestParams(params)).toBe(true);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user