fix: add discord exec approval channel targeting (#16051) (thanks @leonnardo)

This commit is contained in:
Shadow
2026-02-14 12:04:01 -06:00
committed by Shadow
parent 4b9cb46c6e
commit 5ba72bd9bf
6 changed files with 631 additions and 91 deletions

View File

@@ -1,12 +1,106 @@
import { describe, expect, it } from "vitest";
import type { ButtonInteraction, ComponentData } from "@buape/carbon";
import { Routes } from "discord-api-types/v10";
import { beforeEach, describe, expect, it, vi } from "vitest";
import type { DiscordExecApprovalConfig } from "../../config/types.discord.js";
import {
buildExecApprovalCustomId,
extractDiscordChannelId,
parseExecApprovalData,
type ExecApprovalRequest,
DiscordExecApprovalHandler,
ExecApprovalButton,
type ExecApprovalButtonContext,
} from "./exec-approvals.js";
// ─── Mocks ────────────────────────────────────────────────────────────────────
const mockRestPost = vi.hoisted(() => vi.fn());
const mockRestPatch = vi.hoisted(() => vi.fn());
const mockRestDelete = vi.hoisted(() => vi.fn());
vi.mock("../send.shared.js", () => ({
createDiscordClient: () => ({
rest: {
post: mockRestPost,
patch: mockRestPatch,
delete: mockRestDelete,
},
request: (_fn: () => Promise<unknown>, _label: string) => _fn(),
}),
}));
vi.mock("../../gateway/client.js", () => ({
GatewayClient: class {
private params: Record<string, unknown>;
constructor(params: Record<string, unknown>) {
this.params = params;
}
start() {}
stop() {}
async request() {
return { ok: true };
}
},
}));
vi.mock("../../logger.js", () => ({
logDebug: vi.fn(),
logError: vi.fn(),
}));
// ─── Helpers ──────────────────────────────────────────────────────────────────
function createHandler(config: DiscordExecApprovalConfig) {
return new DiscordExecApprovalHandler({
token: "test-token",
accountId: "default",
config,
cfg: {},
});
}
type ExecApprovalHandlerInternals = DiscordExecApprovalHandler & {
pending: Map<
string,
{ discordMessageId: string; discordChannelId: string; timeoutId: NodeJS.Timeout }
>;
requestCache: Map<string, ExecApprovalRequest>;
handleApprovalRequested: (request: ExecApprovalRequest) => Promise<void>;
handleApprovalTimeout: (approvalId: string, source?: "channel" | "dm") => Promise<void>;
};
function getHandlerInternals(handler: DiscordExecApprovalHandler): ExecApprovalHandlerInternals {
return handler as unknown as ExecApprovalHandlerInternals;
}
function clearPendingTimeouts(handler: DiscordExecApprovalHandler) {
const internals = getHandlerInternals(handler);
for (const pending of internals.pending.values()) {
clearTimeout(pending.timeoutId);
}
internals.pending.clear();
}
function createRequest(
overrides: Partial<ExecApprovalRequest["request"]> = {},
): ExecApprovalRequest {
return {
id: "test-id",
request: {
command: "echo hello",
cwd: "/home/user",
host: "gateway",
agentId: "test-agent",
sessionKey: "agent:test-agent:discord:channel:999888777",
...overrides,
},
createdAtMs: Date.now(),
expiresAtMs: Date.now() + 60000,
};
}
// ─── buildExecApprovalCustomId ────────────────────────────────────────────────
describe("buildExecApprovalCustomId", () => {
it("encodes approval id and action", () => {
const customId = buildExecApprovalCustomId("abc-123", "allow-once");
@@ -19,6 +113,8 @@ describe("buildExecApprovalCustomId", () => {
});
});
// ─── parseExecApprovalData ────────────────────────────────────────────────────
describe("parseExecApprovalData", () => {
it("parses valid data", () => {
const result = parseExecApprovalData({ id: "abc-123", action: "allow-once" });
@@ -62,6 +158,8 @@ describe("parseExecApprovalData", () => {
});
});
// ─── roundtrip encoding ───────────────────────────────────────────────────────
describe("roundtrip encoding", () => {
it("encodes and decodes correctly", () => {
const approvalId = "test-approval-with=special;chars&more";
@@ -83,34 +181,51 @@ describe("roundtrip encoding", () => {
});
});
// ─── extractDiscordChannelId ──────────────────────────────────────────────────
describe("extractDiscordChannelId", () => {
it("extracts channel ID from standard session key", () => {
expect(extractDiscordChannelId("agent:main:discord:channel:123456789")).toBe("123456789");
});
it("extracts channel ID from agent session key", () => {
expect(extractDiscordChannelId("agent:test-agent:discord:channel:999888777")).toBe("999888777");
});
it("extracts channel ID from group session key", () => {
expect(extractDiscordChannelId("agent:main:discord:group:222333444")).toBe("222333444");
});
it("returns null for non-discord session key", () => {
expect(extractDiscordChannelId("agent:main:telegram:channel:123456789")).toBeNull();
});
it("returns null for session key without channel segment", () => {
expect(extractDiscordChannelId("agent:main:discord:dm:123456789")).toBeNull();
});
it("returns null for null input", () => {
expect(extractDiscordChannelId(null)).toBeNull();
});
it("returns null for undefined input", () => {
expect(extractDiscordChannelId(undefined)).toBeNull();
});
it("returns null for empty string", () => {
expect(extractDiscordChannelId("")).toBeNull();
});
it("extracts from longer session keys", () => {
expect(extractDiscordChannelId("agent:my-agent:discord:channel:111222333:thread:444555")).toBe(
"111222333",
);
});
});
// ─── DiscordExecApprovalHandler.shouldHandle ──────────────────────────────────
describe("DiscordExecApprovalHandler.shouldHandle", () => {
function createHandler(config: DiscordExecApprovalConfig) {
return new DiscordExecApprovalHandler({
token: "test-token",
accountId: "default",
config,
cfg: {},
});
}
function createRequest(
overrides: Partial<ExecApprovalRequest["request"]> = {},
): ExecApprovalRequest {
return {
id: "test-id",
request: {
command: "echo hello",
cwd: "/home/user",
host: "gateway",
agentId: "test-agent",
sessionKey: "agent:test-agent:discord:123",
...overrides,
},
createdAtMs: Date.now(),
expiresAtMs: Date.now() + 60000,
};
}
it("returns false when disabled", () => {
const handler = createHandler({ enabled: false, approvers: ["123"] });
expect(handler.shouldHandle(createRequest())).toBe(false);
@@ -199,3 +314,316 @@ describe("DiscordExecApprovalHandler.shouldHandle", () => {
).toBe(false);
});
});
// ─── DiscordExecApprovalHandler.getApprovers ──────────────────────────────────
describe("DiscordExecApprovalHandler.getApprovers", () => {
it("returns configured approvers", () => {
const handler = createHandler({ enabled: true, approvers: ["111", "222"] });
expect(handler.getApprovers()).toEqual(["111", "222"]);
});
it("returns empty array when no approvers configured", () => {
const handler = createHandler({ enabled: true, approvers: [] });
expect(handler.getApprovers()).toEqual([]);
});
it("returns empty array when approvers is undefined", () => {
const handler = createHandler({ enabled: true } as DiscordExecApprovalConfig);
expect(handler.getApprovers()).toEqual([]);
});
});
// ─── ExecApprovalButton authorization ─────────────────────────────────────────
describe("ExecApprovalButton", () => {
function createMockHandler(approverIds: string[]) {
const handler = createHandler({
enabled: true,
approvers: approverIds,
});
// Mock resolveApproval to track calls
handler.resolveApproval = vi.fn().mockResolvedValue(true);
return handler;
}
function createMockInteraction(userId: string) {
const reply = vi.fn().mockResolvedValue(undefined);
const update = vi.fn().mockResolvedValue(undefined);
const followUp = vi.fn().mockResolvedValue(undefined);
const interaction = {
userId,
reply,
update,
followUp,
} as unknown as ButtonInteraction;
return { interaction, reply, update, followUp };
}
it("denies unauthorized users with ephemeral message", async () => {
const handler = createMockHandler(["111", "222"]);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, reply, update } = createMockInteraction("999");
const data: ComponentData = { id: "test-approval", action: "allow-once" };
await button.run(interaction, data);
expect(reply).toHaveBeenCalledWith({
content: "⛔ You are not authorized to approve exec requests.",
ephemeral: true,
});
expect(update).not.toHaveBeenCalled();
// oxlint-disable-next-line typescript/unbound-method -- vi.fn() mock
expect(handler.resolveApproval).not.toHaveBeenCalled();
});
it("allows authorized user and resolves approval", async () => {
const handler = createMockHandler(["111", "222"]);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, reply, update } = createMockInteraction("222");
const data: ComponentData = { id: "test-approval", action: "allow-once" };
await button.run(interaction, data);
expect(reply).not.toHaveBeenCalled();
expect(update).toHaveBeenCalledWith({
content: "Submitting decision: **Allowed (once)**...",
components: [],
});
// oxlint-disable-next-line typescript/unbound-method -- vi.fn() mock
expect(handler.resolveApproval).toHaveBeenCalledWith("test-approval", "allow-once");
});
it("shows correct label for allow-always", async () => {
const handler = createMockHandler(["111"]);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, update } = createMockInteraction("111");
const data: ComponentData = { id: "test-approval", action: "allow-always" };
await button.run(interaction, data);
expect(update).toHaveBeenCalledWith({
content: "Submitting decision: **Allowed (always)**...",
components: [],
});
});
it("shows correct label for deny", async () => {
const handler = createMockHandler(["111"]);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, update } = createMockInteraction("111");
const data: ComponentData = { id: "test-approval", action: "deny" };
await button.run(interaction, data);
expect(update).toHaveBeenCalledWith({
content: "Submitting decision: **Denied**...",
components: [],
});
});
it("handles invalid data gracefully", async () => {
const handler = createMockHandler(["111"]);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, update } = createMockInteraction("111");
const data: ComponentData = { id: "", action: "invalid" };
await button.run(interaction, data);
expect(update).toHaveBeenCalledWith({
content: "This approval is no longer valid.",
components: [],
});
// oxlint-disable-next-line typescript/unbound-method -- vi.fn() mock
expect(handler.resolveApproval).not.toHaveBeenCalled();
});
it("follows up with error when resolve fails", async () => {
const handler = createMockHandler(["111"]);
handler.resolveApproval = vi.fn().mockResolvedValue(false);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, followUp } = createMockInteraction("111");
const data: ComponentData = { id: "test-approval", action: "allow-once" };
await button.run(interaction, data);
expect(followUp).toHaveBeenCalledWith({
content:
"Failed to submit approval decision. The request may have expired or already been resolved.",
ephemeral: true,
});
});
it("matches approvers with string coercion", async () => {
// Approvers might be numbers in config
const handler = createHandler({
enabled: true,
approvers: [111 as unknown as string],
});
handler.resolveApproval = vi.fn().mockResolvedValue(true);
const ctx: ExecApprovalButtonContext = { handler };
const button = new ExecApprovalButton(ctx);
const { interaction, update, reply } = createMockInteraction("111");
const data: ComponentData = { id: "test-approval", action: "allow-once" };
await button.run(interaction, data);
// Should match because getApprovers returns [111] and button does String(id) === userId
expect(reply).not.toHaveBeenCalled();
expect(update).toHaveBeenCalled();
});
});
// ─── Target routing (handler config) ──────────────────────────────────────────
describe("DiscordExecApprovalHandler target config", () => {
beforeEach(() => {
mockRestPost.mockReset();
mockRestPatch.mockReset();
mockRestDelete.mockReset();
});
it("defaults target to dm when not specified", () => {
const config: DiscordExecApprovalConfig = {
enabled: true,
approvers: ["123"],
};
// target should be undefined, handler defaults to "dm"
expect(config.target).toBeUndefined();
const handler = createHandler(config);
// Handler should still handle requests (no crash on missing target)
expect(handler.shouldHandle(createRequest())).toBe(true);
});
it("accepts target=channel in config", () => {
const handler = createHandler({
enabled: true,
approvers: ["123"],
target: "channel",
});
expect(handler.shouldHandle(createRequest())).toBe(true);
});
it("accepts target=both in config", () => {
const handler = createHandler({
enabled: true,
approvers: ["123"],
target: "both",
});
expect(handler.shouldHandle(createRequest())).toBe(true);
});
it("accepts target=dm in config", () => {
const handler = createHandler({
enabled: true,
approvers: ["123"],
target: "dm",
});
expect(handler.shouldHandle(createRequest())).toBe(true);
});
});
// ─── Timeout cleanup ─────────────────────────────────────────────────────────
describe("DiscordExecApprovalHandler timeout cleanup", () => {
beforeEach(() => {
mockRestPost.mockReset();
mockRestPatch.mockReset();
mockRestDelete.mockReset();
});
it("cleans up request cache for the exact approval id", async () => {
const handler = createHandler({ enabled: true, approvers: ["123"] });
const internals = getHandlerInternals(handler);
const requestA = { ...createRequest(), id: "abc" };
const requestB = { ...createRequest(), id: "abc2" };
internals.requestCache.set("abc", requestA);
internals.requestCache.set("abc2", requestB);
const timeoutIdA = setTimeout(() => {}, 0);
const timeoutIdB = setTimeout(() => {}, 0);
clearTimeout(timeoutIdA);
clearTimeout(timeoutIdB);
internals.pending.set("abc:dm", {
discordMessageId: "m1",
discordChannelId: "c1",
timeoutId: timeoutIdA,
});
internals.pending.set("abc2:dm", {
discordMessageId: "m2",
discordChannelId: "c2",
timeoutId: timeoutIdB,
});
await internals.handleApprovalTimeout("abc", "dm");
expect(internals.pending.has("abc:dm")).toBe(false);
expect(internals.requestCache.has("abc")).toBe(false);
expect(internals.requestCache.has("abc2")).toBe(true);
clearPendingTimeouts(handler);
});
});
// ─── Delivery routing ────────────────────────────────────────────────────────
describe("DiscordExecApprovalHandler delivery routing", () => {
beforeEach(() => {
mockRestPost.mockReset();
mockRestPatch.mockReset();
mockRestDelete.mockReset();
});
it("falls back to DM delivery when channel target has no channel id", async () => {
const handler = createHandler({
enabled: true,
approvers: ["123"],
target: "channel",
});
const internals = getHandlerInternals(handler);
mockRestPost.mockImplementation(async (route: string) => {
if (route === Routes.userChannels()) {
return { id: "dm-1" };
}
if (route === Routes.channelMessages("dm-1")) {
return { id: "msg-1", channel_id: "dm-1" };
}
return { id: "msg-unknown" };
});
const request = createRequest({ sessionKey: "agent:main:discord:dm:123" });
await internals.handleApprovalRequested(request);
expect(mockRestPost).toHaveBeenCalledTimes(2);
expect(mockRestPost).toHaveBeenCalledWith(Routes.userChannels(), {
body: { recipient_id: "123" },
});
expect(mockRestPost).toHaveBeenCalledWith(
Routes.channelMessages("dm-1"),
expect.objectContaining({
body: expect.objectContaining({
embeds: expect.any(Array),
components: expect.any(Array),
}),
}),
);
clearPendingTimeouts(handler);
});
});

View File

@@ -19,6 +19,16 @@ const EXEC_APPROVAL_KEY = "execapproval";
export type { ExecApprovalRequest, ExecApprovalResolved };
/** Extract Discord channel ID from a session key like "agent:main:discord:channel:123456789" */
export function extractDiscordChannelId(sessionKey?: string | null): string | null {
if (!sessionKey) {
return null;
}
// Session key format: agent:<id>:discord:channel:<channelId> or agent:<id>:discord:group:<channelId>
const match = sessionKey.match(/discord:(?:channel|group):(\d+)/);
return match ? match[1] : null;
}
type PendingApproval = {
discordMessageId: string;
discordChannelId: string;
@@ -348,70 +358,122 @@ export class DiscordExecApprovalHandler {
},
];
const approvers = this.opts.config.approvers ?? [];
const target = this.opts.config.target ?? "dm";
const sendToDm = target === "dm" || target === "both";
const sendToChannel = target === "channel" || target === "both";
let fallbackToDm = false;
for (const approver of approvers) {
const userId = String(approver);
try {
// Create DM channel
const dmChannel = (await discordRequest(
() =>
rest.post(Routes.userChannels(), {
body: { recipient_id: userId },
}) as Promise<{ id: string }>,
"dm-channel",
)) as { id: string };
// Send to originating channel if configured
if (sendToChannel) {
const channelId = extractDiscordChannelId(request.request.sessionKey);
if (channelId) {
try {
const message = (await discordRequest(
() =>
rest.post(Routes.channelMessages(channelId), {
body: {
embeds: [embed],
components,
},
}) as Promise<{ id: string; channel_id: string }>,
"send-approval-channel",
)) as { id: string; channel_id: string };
if (!dmChannel?.id) {
logError(`discord exec approvals: failed to create DM for user ${userId}`);
continue;
if (message?.id) {
const timeoutMs = Math.max(0, request.expiresAtMs - Date.now());
const timeoutId = setTimeout(() => {
void this.handleApprovalTimeout(request.id, "channel");
}, timeoutMs);
this.pending.set(`${request.id}:channel`, {
discordMessageId: message.id,
discordChannelId: channelId,
timeoutId,
});
logDebug(`discord exec approvals: sent approval ${request.id} to channel ${channelId}`);
}
} catch (err) {
logError(`discord exec approvals: failed to send to channel: ${String(err)}`);
}
// Send message with embed and buttons
const message = (await discordRequest(
() =>
rest.post(Routes.channelMessages(dmChannel.id), {
body: {
embeds: [embed],
components,
},
}) as Promise<{ id: string; channel_id: string }>,
"send-approval",
)) as { id: string; channel_id: string };
if (!message?.id) {
logError(`discord exec approvals: failed to send message to user ${userId}`);
continue;
} else {
if (!sendToDm) {
logError(
`discord exec approvals: target is "channel" but could not extract channel id from session key "${request.request.sessionKey ?? "(none)"}" — falling back to DM delivery for approval ${request.id}`,
);
fallbackToDm = true;
} else {
logDebug(`discord exec approvals: could not extract channel id from session key`);
}
}
}
// Set up timeout
const timeoutMs = Math.max(0, request.expiresAtMs - Date.now());
const timeoutId = setTimeout(() => {
void this.handleApprovalTimeout(request.id);
}, timeoutMs);
// Send to approver DMs if configured (or as fallback when channel extraction fails)
if (sendToDm || fallbackToDm) {
const approvers = this.opts.config.approvers ?? [];
this.pending.set(request.id, {
discordMessageId: message.id,
discordChannelId: dmChannel.id,
timeoutId,
});
for (const approver of approvers) {
const userId = String(approver);
try {
// Create DM channel
const dmChannel = (await discordRequest(
() =>
rest.post(Routes.userChannels(), {
body: { recipient_id: userId },
}) as Promise<{ id: string }>,
"dm-channel",
)) as { id: string };
logDebug(`discord exec approvals: sent approval ${request.id} to user ${userId}`);
} catch (err) {
logError(`discord exec approvals: failed to notify user ${userId}: ${String(err)}`);
if (!dmChannel?.id) {
logError(`discord exec approvals: failed to create DM for user ${userId}`);
continue;
}
// Send message with embed and buttons
const message = (await discordRequest(
() =>
rest.post(Routes.channelMessages(dmChannel.id), {
body: {
embeds: [embed],
components,
},
}) as Promise<{ id: string; channel_id: string }>,
"send-approval",
)) as { id: string; channel_id: string };
if (!message?.id) {
logError(`discord exec approvals: failed to send message to user ${userId}`);
continue;
}
// Clear any existing pending DM entry to avoid timeout leaks
const existingDm = this.pending.get(`${request.id}:dm`);
if (existingDm) {
clearTimeout(existingDm.timeoutId);
}
// Set up timeout
const timeoutMs = Math.max(0, request.expiresAtMs - Date.now());
const timeoutId = setTimeout(() => {
void this.handleApprovalTimeout(request.id, "dm");
}, timeoutMs);
this.pending.set(`${request.id}:dm`, {
discordMessageId: message.id,
discordChannelId: dmChannel.id,
timeoutId,
});
logDebug(`discord exec approvals: sent approval ${request.id} to user ${userId}`);
} catch (err) {
logError(`discord exec approvals: failed to notify user ${userId}: ${String(err)}`);
}
}
}
}
private async handleApprovalResolved(resolved: ExecApprovalResolved): Promise<void> {
const pending = this.pending.get(resolved.id);
if (!pending) {
return;
}
clearTimeout(pending.timeoutId);
this.pending.delete(resolved.id);
// Clean up all pending entries for this approval (channel + dm)
const request = this.requestCache.get(resolved.id);
this.requestCache.delete(resolved.id);
@@ -421,29 +483,50 @@ export class DiscordExecApprovalHandler {
logDebug(`discord exec approvals: resolved ${resolved.id} with ${resolved.decision}`);
await this.finalizeMessage(
pending.discordChannelId,
pending.discordMessageId,
formatResolvedEmbed(request, resolved.decision, resolved.resolvedBy),
);
const resolvedEmbed = formatResolvedEmbed(request, resolved.decision, resolved.resolvedBy);
for (const suffix of [":channel", ":dm", ""]) {
const key = `${resolved.id}${suffix}`;
const pending = this.pending.get(key);
if (!pending) {
continue;
}
clearTimeout(pending.timeoutId);
this.pending.delete(key);
await this.finalizeMessage(pending.discordChannelId, pending.discordMessageId, resolvedEmbed);
}
}
private async handleApprovalTimeout(approvalId: string): Promise<void> {
const pending = this.pending.get(approvalId);
private async handleApprovalTimeout(
approvalId: string,
source?: "channel" | "dm",
): Promise<void> {
const key = source ? `${approvalId}:${source}` : approvalId;
const pending = this.pending.get(key);
if (!pending) {
return;
}
this.pending.delete(approvalId);
this.pending.delete(key);
const request = this.requestCache.get(approvalId);
this.requestCache.delete(approvalId);
// Only clean up requestCache if no other pending entries exist for this approval
const hasOtherPending =
this.pending.has(`${approvalId}:channel`) ||
this.pending.has(`${approvalId}:dm`) ||
this.pending.has(approvalId);
if (!hasOtherPending) {
this.requestCache.delete(approvalId);
}
if (!request) {
return;
}
logDebug(`discord exec approvals: timeout for ${approvalId}`);
logDebug(`discord exec approvals: timeout for ${approvalId} (${source ?? "default"})`);
await this.finalizeMessage(
pending.discordChannelId,
@@ -524,6 +607,11 @@ export class DiscordExecApprovalHandler {
return false;
}
}
/** Return the list of configured approver IDs. */
getApprovers(): Array<string | number> {
return this.opts.config.approvers ?? [];
}
}
export type ExecApprovalButtonContext = {
@@ -555,6 +643,21 @@ export class ExecApprovalButton extends Button {
return;
}
// Verify the user is an authorized approver
const approvers = this.ctx.handler.getApprovers();
const userId = interaction.userId;
if (!approvers.some((id) => String(id) === userId)) {
try {
await interaction.reply({
content: "⛔ You are not authorized to approve exec requests.",
ephemeral: true,
});
} catch {
// Interaction may have expired
}
return;
}
const decisionLabel =
parsed.action === "allow-once"
? "Allowed (once)"