From d60a5f7dd41e7ffcc5bebc8b9361a12a8c0172fe Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 9 Aug 2026 08:30:53 -0700 Subject: [PATCH] refactor(plugins): consolidate delivery fixtures (#114464) --- .../send.sends-basic-channel-messages.test.ts | 334 ++++++---------- extensions/matrix/src/matrix/accounts.test.ts | 231 +++++------ .../slack/src/message-action-dispatch.test.ts | 358 ++++++------------ extensions/slack/src/monitor/media.test.ts | 36 +- 4 files changed, 339 insertions(+), 620 deletions(-) diff --git a/extensions/discord/src/send.sends-basic-channel-messages.test.ts b/extensions/discord/src/send.sends-basic-channel-messages.test.ts index b949f67a3ad2..78af5bc0cee8 100644 --- a/extensions/discord/src/send.sends-basic-channel-messages.test.ts +++ b/extensions/discord/src/send.sends-basic-channel-messages.test.ts @@ -646,9 +646,22 @@ describe("sendMessageDiscord", () => { expect(requireRestBody(postMock).content).toBe("hello"); }); - it("adds missing permission hints on 50013", async () => { + it.each([ + { + name: "adds missing permission hints on 50013", + permissions: PermissionFlagsBits.ViewChannel, + expectedErrors: [/missing permissions/i, /SendMessages/], + }, + { + name: "keeps 50013 context when permission probe finds baseline permissions", + permissions: PermissionFlagsBits.ViewChannel | PermissionFlagsBits.SendMessages, + expectedErrors: [ + /permission probe did not identify missing ViewChannel\/SendMessages/, + /code=50013 status=403/, + ], + }, + ])("$name", async ({ permissions, expectedErrors }) => { const { rest, postMock, getMock } = makeDiscordRest(); - const perms = PermissionFlagsBits.ViewChannel; const apiError = Object.assign(new Error("Missing Permissions"), { code: 50013, status: 403, @@ -665,7 +678,7 @@ describe("sendMessageDiscord", () => { .mockResolvedValueOnce({ id: "bot1" }) .mockResolvedValueOnce({ id: "guild1", - roles: [{ id: "guild1", permissions: perms.toString() }], + roles: [{ id: "guild1", permissions: permissions.toString() }], }) .mockResolvedValueOnce({ roles: [] }); @@ -675,43 +688,9 @@ describe("sendMessageDiscord", () => { } catch (err) { error = err; } - expect(String(error)).toMatch(/missing permissions/i); - expect(String(error)).toMatch(/SendMessages/); - }); - - it("keeps 50013 context when permission probe finds baseline permissions", async () => { - const { rest, postMock, getMock } = makeDiscordRest(); - const perms = PermissionFlagsBits.ViewChannel | PermissionFlagsBits.SendMessages; - const apiError = Object.assign(new Error("Missing Permissions"), { - code: 50013, - status: 403, - }); - postMock.mockRejectedValueOnce(apiError); - getMock - .mockResolvedValueOnce({ type: ChannelType.GuildText }) - .mockResolvedValueOnce({ - id: "789", - guild_id: "guild1", - type: 0, - permission_overwrites: [], - }) - .mockResolvedValueOnce({ id: "bot1" }) - .mockResolvedValueOnce({ - id: "guild1", - roles: [{ id: "guild1", permissions: perms.toString() }], - }) - .mockResolvedValueOnce({ roles: [] }); - - let error: unknown; - try { - await sendMessageDiscord("channel:789", "hello", { rest, token: "t", cfg: DISCORD_TEST_CFG }); - } catch (err) { - error = err; + for (const expectedError of expectedErrors) { + expect(String(error)).toMatch(expectedError); } - expect(String(error)).toMatch( - /permission probe did not identify missing ViewChannel\/SendMessages/, - ); - expect(String(error)).toMatch(/code=50013 status=403/); }); it("uploads media attachments", async () => { @@ -959,43 +938,45 @@ describe("sendMessageDiscord", () => { }); }); - it("preserves reply reference across all text chunks by default", async () => { - const { firstBody, secondBody } = await sendChunkedReplyAndCollectBodies({ - text: "a".repeat(2001), - }); + it.each([ + { + name: "preserves reply reference across all text chunks by default", + params: { text: "a".repeat(2001) }, + expectsSecondReply: true, + }, + { + name: "limits reply reference to the first text chunk when requested", + params: { text: "a".repeat(2001), replyScope: "first" as const }, + expectsSecondReply: false, + checksReceipt: true, + }, + { + name: "preserves reply reference for follow-up text chunks after media caption split by default", + params: { text: "a".repeat(2500), mediaUrl: "file:///tmp/photo.jpg" }, + expectsSecondReply: true, + }, + { + name: "limits media caption reply reference to the first physical message when requested", + params: { + text: "a".repeat(2500), + mediaUrl: "file:///tmp/photo.jpg", + replyScope: "first" as const, + }, + expectsSecondReply: false, + }, + ])("$name", async ({ params, expectsSecondReply, checksReceipt }) => { + const { firstBody, secondBody, result } = await sendChunkedReplyAndCollectBodies(params); expectReplyReference(firstBody, "orig-123"); - expectReplyReference(secondBody, "orig-123"); - }); - - it("limits reply reference to the first text chunk when requested", async () => { - const { firstBody, secondBody, result } = await sendChunkedReplyAndCollectBodies({ - text: "a".repeat(2001), - replyScope: "first", - }); - expectReplyReference(firstBody, "orig-123"); - expectNoReplyReference(secondBody); - expect(result.receipt.replyToId).toBe("orig-123"); - expect(result.receipt.parts.map((part) => part.replyToId)).toEqual(["orig-123", undefined]); - expect(() => JSON.stringify(result.receipt)).not.toThrow(); - }); - - it("preserves reply reference for follow-up text chunks after media caption split by default", async () => { - const { firstBody, secondBody } = await sendChunkedReplyAndCollectBodies({ - text: "a".repeat(2500), - mediaUrl: "file:///tmp/photo.jpg", - }); - expectReplyReference(firstBody, "orig-123"); - expectReplyReference(secondBody, "orig-123"); - }); - - it("limits media caption reply reference to the first physical message when requested", async () => { - const { firstBody, secondBody } = await sendChunkedReplyAndCollectBodies({ - text: "a".repeat(2500), - mediaUrl: "file:///tmp/photo.jpg", - replyScope: "first", - }); - expectReplyReference(firstBody, "orig-123"); - expectNoReplyReference(secondBody); + if (expectsSecondReply) { + expectReplyReference(secondBody, "orig-123"); + } else { + expectNoReplyReference(secondBody); + } + if (checksReceipt) { + expect(result.receipt.replyToId).toBe("orig-123"); + expect(result.receipt.parts.map((part) => part.replyToId)).toEqual(["orig-123", undefined]); + expect(() => JSON.stringify(result.receipt)).not.toThrow(); + } }); }); @@ -1004,31 +985,23 @@ describe("reactMessageDiscord", () => { vi.clearAllMocks(); }); - it("reacts with unicode emoji", async () => { + it.each([ + { name: "reacts with unicode emoji", emoji: "✅", encoded: "%E2%9C%85" }, + { + name: "normalizes variation selectors in unicode emoji", + emoji: "⭐️", + encoded: "%E2%AD%90", + }, + { + name: "reacts with custom emoji syntax", + emoji: "<:party_blob:123>", + encoded: "party_blob%3A123", + }, + ])("$name", async ({ emoji, encoded }) => { const { rest, putMock } = makeDiscordRest(); - await reactMessageDiscord("chan1", "msg1", "✅", { rest, token: "t", cfg: DISCORD_TEST_CFG }); + await reactMessageDiscord("chan1", "msg1", emoji, { rest, token: "t", cfg: DISCORD_TEST_CFG }); expect(putMock).toHaveBeenCalledWith( - Routes.channelMessageOwnReaction("chan1", "msg1", "%E2%9C%85"), - ); - }); - - it("normalizes variation selectors in unicode emoji", async () => { - const { rest, putMock } = makeDiscordRest(); - await reactMessageDiscord("chan1", "msg1", "⭐️", { rest, token: "t", cfg: DISCORD_TEST_CFG }); - expect(putMock).toHaveBeenCalledWith( - Routes.channelMessageOwnReaction("chan1", "msg1", "%E2%AD%90"), - ); - }); - - it("reacts with custom emoji syntax", async () => { - const { rest, putMock } = makeDiscordRest(); - await reactMessageDiscord("chan1", "msg1", "<:party_blob:123>", { - rest, - token: "t", - cfg: DISCORD_TEST_CFG, - }); - expect(putMock).toHaveBeenCalledWith( - Routes.channelMessageOwnReaction("chan1", "msg1", "party_blob%3A123"), + Routes.channelMessageOwnReaction("chan1", "msg1", encoded), ); }); }); @@ -1379,142 +1352,79 @@ describe("fetchChannelPermissionsDiscord", () => { ).resolves.toBe(true); }); - it("uses parent ViewChannel permissions for a public thread", async () => { + it.each([ + { + name: "uses parent ViewChannel permissions for a public thread", + type: ChannelType.GuildPublicThread, + overwrites: [{ id: "user1", deny: PermissionFlagsBits.ViewChannel.toString(), allow: "0" }], + permissions: PermissionFlagsBits.ViewChannel, + membership: "none", + expected: false, + expectedCalls: 4, + }, + { + name: "requires private-thread membership after parent ViewChannel permission", + type: ChannelType.GuildPrivateThread, + overwrites: [], + permissions: PermissionFlagsBits.ViewChannel, + membership: "member", + expected: true, + }, + { + name: "fails closed when a user is not a private-thread member", + type: ChannelType.GuildPrivateThread, + overwrites: [], + permissions: PermissionFlagsBits.ViewChannel, + membership: "missing", + expected: false, + }, + { + name: "allows private-thread moderators without explicit membership", + type: ChannelType.GuildPrivateThread, + overwrites: [], + permissions: PermissionFlagsBits.ViewChannel | PermissionFlagsBits.ManageThreads, + membership: "none", + expected: true, + expectedCalls: 4, + }, + ])("$name", async ({ type, overwrites, permissions, membership, expected, expectedCalls }) => { const { rest, getMock } = makeDiscordRest(); getMock .mockResolvedValueOnce({ id: "thread1", guild_id: "guild1", parent_id: "parent1", - type: ChannelType.GuildPublicThread, + type, }) .mockResolvedValueOnce({ id: "parent1", guild_id: "guild1", type: ChannelType.GuildText, - permission_overwrites: [ - { - id: "user1", - deny: PermissionFlagsBits.ViewChannel.toString(), - allow: "0", - }, - ], + permission_overwrites: overwrites, }) .mockResolvedValueOnce({ id: "guild1", - roles: [{ id: "guild1", permissions: PermissionFlagsBits.ViewChannel.toString() }], + roles: [{ id: "guild1", permissions: permissions.toString() }], }) .mockResolvedValueOnce({ roles: [] }); - + if (membership === "member") { + getMock.mockResolvedValueOnce({ id: "thread1", user_id: "user1" }); + } else if (membership === "missing") { + getMock.mockRejectedValueOnce(new Error("404 Unknown Member")); + } await expect( canViewDiscordGuildChannel("guild1", "thread1", "user1", { rest, token: "t", cfg: DISCORD_TEST_CFG, }), - ).resolves.toBe(false); - expect(getMock).toHaveBeenCalledTimes(4); - }); - - it("requires private-thread membership after parent ViewChannel permission", async () => { - const { rest, getMock } = makeDiscordRest(); - getMock - .mockResolvedValueOnce({ - id: "thread1", - guild_id: "guild1", - parent_id: "parent1", - type: ChannelType.GuildPrivateThread, - }) - .mockResolvedValueOnce({ - id: "parent1", - guild_id: "guild1", - type: ChannelType.GuildText, - permission_overwrites: [], - }) - .mockResolvedValueOnce({ - id: "guild1", - roles: [{ id: "guild1", permissions: PermissionFlagsBits.ViewChannel.toString() }], - }) - .mockResolvedValueOnce({ roles: [] }) - .mockResolvedValueOnce({ id: "thread1", user_id: "user1" }); - - await expect( - canViewDiscordGuildChannel("guild1", "thread1", "user1", { - rest, - token: "t", - cfg: DISCORD_TEST_CFG, - }), - ).resolves.toBe(true); - expect(getMock).toHaveBeenLastCalledWith(Routes.threadMembers("thread1", "user1")); - }); - - it("fails closed when a user is not a private-thread member", async () => { - const { rest, getMock } = makeDiscordRest(); - getMock - .mockResolvedValueOnce({ - id: "thread1", - guild_id: "guild1", - parent_id: "parent1", - type: ChannelType.GuildPrivateThread, - }) - .mockResolvedValueOnce({ - id: "parent1", - guild_id: "guild1", - type: ChannelType.GuildText, - permission_overwrites: [], - }) - .mockResolvedValueOnce({ - id: "guild1", - roles: [{ id: "guild1", permissions: PermissionFlagsBits.ViewChannel.toString() }], - }) - .mockResolvedValueOnce({ roles: [] }) - .mockRejectedValueOnce(new Error("404 Unknown Member")); - - await expect( - canViewDiscordGuildChannel("guild1", "thread1", "user1", { - rest, - token: "t", - cfg: DISCORD_TEST_CFG, - }), - ).resolves.toBe(false); - }); - - it("allows private-thread moderators without explicit membership", async () => { - const { rest, getMock } = makeDiscordRest(); - getMock - .mockResolvedValueOnce({ - id: "thread1", - guild_id: "guild1", - parent_id: "parent1", - type: ChannelType.GuildPrivateThread, - }) - .mockResolvedValueOnce({ - id: "parent1", - guild_id: "guild1", - type: ChannelType.GuildText, - permission_overwrites: [], - }) - .mockResolvedValueOnce({ - id: "guild1", - roles: [ - { - id: "guild1", - permissions: ( - PermissionFlagsBits.ViewChannel | PermissionFlagsBits.ManageThreads - ).toString(), - }, - ], - }) - .mockResolvedValueOnce({ roles: [] }); - - await expect( - canViewDiscordGuildChannel("guild1", "thread1", "user1", { - rest, - token: "t", - cfg: DISCORD_TEST_CFG, - }), - ).resolves.toBe(true); - expect(getMock).toHaveBeenCalledTimes(4); + ).resolves.toBe(expected); + if (expectedCalls !== undefined) { + expect(getMock).toHaveBeenCalledTimes(expectedCalls); + } + if (membership === "member") { + expect(getMock).toHaveBeenLastCalledWith(Routes.threadMembers("thread1", "user1")); + } }); it("fails closed when the channel belongs to a different guild", async () => { diff --git a/extensions/matrix/src/matrix/accounts.test.ts b/extensions/matrix/src/matrix/accounts.test.ts index ac866521cae9..97b18c85b01f 100644 --- a/extensions/matrix/src/matrix/accounts.test.ts +++ b/extensions/matrix/src/matrix/accounts.test.ts @@ -189,79 +189,46 @@ describe("resolveMatrixAccount", () => { expect(account.configured).toBe(true); }); - it("treats SecretRef access-token config as configured", () => { - const cfg: CoreConfig = { - channels: { - matrix: { - homeserver: "https://matrix.example.org", - accessToken: { source: "file", provider: "matrix-file", id: "value" }, - }, + it.each([ + { + name: "treats SecretRef access-token config as configured", + matrix: { + homeserver: "https://matrix.example.org", + accessToken: { source: "file", provider: "matrix-file", id: "value" }, }, - secrets: { - providers: { - "matrix-file": { - source: "file", - path: "/tmp/matrix-token", + path: "/tmp/matrix-token", + }, + { + name: "treats accounts.default SecretRef access-token config as configured", + matrix: { + accounts: { + default: { + homeserver: "https://matrix.example.org", + accessToken: { source: "file", provider: "matrix-file", id: "value" }, }, }, }, - }; - - const account = resolveMatrixAccount({ cfg }); - expect(account.configured).toBe(true); - }); - - it("treats accounts.default SecretRef access-token config as configured", () => { - const cfg: CoreConfig = { - channels: { - matrix: { - accounts: { - default: { - homeserver: "https://matrix.example.org", - accessToken: { source: "file", provider: "matrix-file", id: "value" }, - }, + path: "/tmp/matrix-token", + }, + { + name: "treats accounts.default SecretRef password config as configured", + matrix: { + accounts: { + default: { + homeserver: "https://matrix.example.org", + userId: "@bot:example.org", + password: { source: "file", provider: "matrix-file", id: "value" }, }, }, }, - secrets: { - providers: { - "matrix-file": { - source: "file", - path: "/tmp/matrix-token", - }, - }, - }, - }; - - const account = resolveMatrixAccount({ cfg }); - expect(account.configured).toBe(true); - }); - - it("treats accounts.default SecretRef password config as configured", () => { - const cfg: CoreConfig = { - channels: { - matrix: { - accounts: { - default: { - homeserver: "https://matrix.example.org", - userId: "@bot:example.org", - password: { source: "file", provider: "matrix-file", id: "value" }, - }, - }, - }, - }, - secrets: { - providers: { - "matrix-file": { - source: "file", - path: "/tmp/matrix-password", - }, - }, - }, - }; - - const account = resolveMatrixAccount({ cfg }); - expect(account.configured).toBe(true); + path: "/tmp/matrix-password", + }, + ])("$name", ({ matrix, path }) => { + const cfg = { + channels: { matrix }, + secrets: { providers: { "matrix-file": { source: "file", path } } }, + } as CoreConfig; + expect(resolveMatrixAccount({ cfg }).configured).toBe(true); }); it("requires userId + password when no access token is set", () => { @@ -621,26 +588,33 @@ describe("resolveMatrixAccount", () => { }); }); - it("filters channel-level groups by room account in multi-account setups", () => { - expectMultiAccountMatrixScopedEntries(createMatrixScopedEntriesConfig("groups"), "groups"); - }); - - it("filters channel-level groups when the default account is configured at the top level", () => { - expectTopLevelDefaultMatrixScopedEntries( - createMatrixTopLevelDefaultScopedEntriesConfig("groups"), - "groups", - ); - }); - - it("filters legacy channel-level rooms by room account in multi-account setups", () => { - expectMultiAccountMatrixScopedEntries(createMatrixScopedEntriesConfig("rooms"), "rooms"); - }); - - it("filters legacy channel-level rooms when the default account is configured at the top level", () => { - expectTopLevelDefaultMatrixScopedEntries( - createMatrixTopLevelDefaultScopedEntriesConfig("rooms"), - "rooms", - ); + it.each([ + { + name: "filters channel-level groups by room account in multi-account setups", + scopeKey: "groups", + createConfig: createMatrixScopedEntriesConfig, + expectEntries: expectMultiAccountMatrixScopedEntries, + }, + { + name: "filters channel-level groups when the default account is configured at the top level", + scopeKey: "groups", + createConfig: createMatrixTopLevelDefaultScopedEntriesConfig, + expectEntries: expectTopLevelDefaultMatrixScopedEntries, + }, + { + name: "filters legacy channel-level rooms by room account in multi-account setups", + scopeKey: "rooms", + createConfig: createMatrixScopedEntriesConfig, + expectEntries: expectMultiAccountMatrixScopedEntries, + }, + { + name: "filters legacy channel-level rooms when the default account is configured at the top level", + scopeKey: "rooms", + createConfig: createMatrixTopLevelDefaultScopedEntriesConfig, + expectEntries: expectTopLevelDefaultMatrixScopedEntries, + }, + ] as const)("$name", ({ scopeKey, createConfig, expectEntries }) => { + expectEntries(createConfig(scopeKey), scopeKey); }); it("honors injected env when scoping room entries in multi-account setups", () => { @@ -682,11 +656,20 @@ describe("resolveMatrixAccount", () => { }); }); - it("keeps scoped groups bound to their account even when only one account is active", () => { + it.each([ + { + name: "keeps scoped groups bound to their account even when only one account is active", + scopeKey: "groups", + }, + { + name: "keeps scoped legacy rooms bound to their account even when only one account is active", + scopeKey: "rooms", + }, + ] as const)("$name", ({ scopeKey }) => { const cfg = { channels: { matrix: { - groups: { + [scopeKey]: { "!default-room:example.org": { enabled: true, account: "default", @@ -705,22 +688,27 @@ describe("resolveMatrixAccount", () => { }, } as unknown as CoreConfig; - expect(resolveMatrixAccount({ cfg, accountId: "ops" }).config.groups).toEqual({ + expect(resolveMatrixAccount({ cfg, accountId: "ops" }).config[scopeKey]).toEqual({ "!shared-room:example.org": { enabled: true, }, }); }); - it("keeps scoped legacy rooms bound to their account even when only one account is active", () => { + it.each([ + { + name: "lets an account clear inherited groups with an explicit empty map", + scopeKey: "groups", + }, + { + name: "lets an account clear inherited legacy rooms with an explicit empty map", + scopeKey: "rooms", + }, + ] as const)("$name", ({ scopeKey }) => { const cfg = { channels: { matrix: { - rooms: { - "!default-room:example.org": { - enabled: true, - account: "default", - }, + [scopeKey]: { "!shared-room:example.org": { enabled: true, }, @@ -729,62 +717,13 @@ describe("resolveMatrixAccount", () => { ops: { homeserver: "https://matrix.example.org", accessToken: "ops-token", + [scopeKey]: {}, }, }, }, }, } as unknown as CoreConfig; - expect(resolveMatrixAccount({ cfg, accountId: "ops" }).config.rooms).toEqual({ - "!shared-room:example.org": { - enabled: true, - }, - }); - }); - - it("lets an account clear inherited groups with an explicit empty map", () => { - const cfg = { - channels: { - matrix: { - groups: { - "!shared-room:example.org": { - enabled: true, - }, - }, - accounts: { - ops: { - homeserver: "https://matrix.example.org", - accessToken: "ops-token", - groups: {}, - }, - }, - }, - }, - } as unknown as CoreConfig; - - expect(resolveMatrixAccount({ cfg, accountId: "ops" }).config.groups).toBeUndefined(); - }); - - it("lets an account clear inherited legacy rooms with an explicit empty map", () => { - const cfg = { - channels: { - matrix: { - rooms: { - "!shared-room:example.org": { - enabled: true, - }, - }, - accounts: { - ops: { - homeserver: "https://matrix.example.org", - accessToken: "ops-token", - rooms: {}, - }, - }, - }, - }, - } as unknown as CoreConfig; - - expect(resolveMatrixAccount({ cfg, accountId: "ops" }).config.rooms).toBeUndefined(); + expect(resolveMatrixAccount({ cfg, accountId: "ops" }).config[scopeKey]).toBeUndefined(); }); }); diff --git a/extensions/slack/src/message-action-dispatch.test.ts b/extensions/slack/src/message-action-dispatch.test.ts index e80873c863de..74e0f650c14d 100644 --- a/extensions/slack/src/message-action-dispatch.test.ts +++ b/extensions/slack/src/message-action-dispatch.test.ts @@ -642,118 +642,111 @@ describe("handleSlackMessageAction", () => { expectForwardedCfg(invoke, cfg); }); - it("passes replyBroadcast through for Slack thread sends", async () => { + it.each([ + { + name: "passes replyBroadcast through for Slack thread sends", + params: { + to: "channel:C1", + message: "Visible from the channel", + threadId: "111.222", + replyBroadcast: true, + }, + expected: { + content: "Visible from the channel", + threadTs: "111.222", + replyBroadcast: true, + }, + }, + { + name: "passes topLevel through so same-channel Slack sends can suppress thread inheritance", + params: { + to: "channel:C1", + message: "Visible in the parent channel", + topLevel: true, + }, + expected: { content: "Visible in the parent channel", threadTs: undefined, topLevel: true }, + }, + { + name: "treats threadId null as a Slack top-level send request", + params: { + to: "channel:C1", + message: "Visible in the parent channel", + threadId: null, + }, + expected: { content: "Visible in the parent channel", threadTs: undefined, topLevel: true }, + }, + ])("$name", async ({ params, expected }) => { const invoke = createInvokeSpy(); const cfg = slackConfig(); - await handleSlackMessageAction({ providerId: "slack", - ctx: { - action: "send", - cfg, - params: { - to: "channel:C1", - message: "Visible from the channel", - threadId: "111.222", - replyBroadcast: true, - }, - } as never, + ctx: { action: "send", cfg, params } as never, invoke: invoke as never, }); - const action = firstAction(invoke); - expect(action.action).toBe("sendMessage"); - expect(action.to).toBe("channel:C1"); - expect(action.content).toBe("Visible from the channel"); - expect(action.threadTs).toBe("111.222"); - expect(action.replyBroadcast).toBe(true); + expect(action).toMatchObject({ action: "sendMessage", to: "channel:C1", ...expected }); + expect(action.threadTs).toBe(expected.threadTs); expectForwardedCfg(invoke, cfg); expectNoForwardedToolContext(invoke); }); - it("passes topLevel through so same-channel Slack sends can suppress thread inheritance", async () => { + it.each([ + { + name: "maps upload-file to the internal uploadFile action", + params: { + to: "user:U1", + filePath: "/tmp/report.png", + initialComment: "fresh build", + filename: "build.png", + title: "Build Screenshot", + threadId: "111.222", + }, + expected: { + to: "user:U1", + filePath: "/tmp/report.png", + initialComment: "fresh build", + filename: "build.png", + title: "Build Screenshot", + threadTs: "111.222", + }, + }, + { + name: "maps upload-file aliases to upload params", + params: { + channelId: "C1", + media: "/tmp/chart.png", + message: "chart attached", + replyTo: "333.444", + }, + expected: { + to: "C1", + filePath: "/tmp/chart.png", + initialComment: "chart attached", + threadTs: "333.444", + }, + }, + { + name: "maps upload-file path alias to filePath", + params: { + to: "channel:C1", + path: "/tmp/report.txt", + initialComment: "path alias", + }, + expected: { + to: "channel:C1", + filePath: "/tmp/report.txt", + initialComment: "path alias", + }, + }, + ])("$name", async ({ params, expected }) => { const invoke = createInvokeSpy(); const cfg = slackConfig(); - await handleSlackMessageAction({ providerId: "slack", - ctx: { - action: "send", - cfg, - params: { - to: "channel:C1", - message: "Visible in the parent channel", - topLevel: true, - }, - } as never, + ctx: { action: "upload-file", cfg, params } as never, invoke: invoke as never, }); - - const action = firstAction(invoke); - expect(action.action).toBe("sendMessage"); - expect(action.to).toBe("channel:C1"); - expect(action.content).toBe("Visible in the parent channel"); - expect(action.threadTs).toBeUndefined(); - expect(action.topLevel).toBe(true); - expectForwardedCfg(invoke, cfg); - expectNoForwardedToolContext(invoke); - }); - - it("treats threadId null as a Slack top-level send request", async () => { - const invoke = createInvokeSpy(); - const cfg = slackConfig(); - - await handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "send", - cfg, - params: { - to: "channel:C1", - message: "Visible in the parent channel", - threadId: null, - }, - } as never, - invoke: invoke as never, - }); - - const action = firstAction(invoke); - expect(action.action).toBe("sendMessage"); - expect(action.threadTs).toBeUndefined(); - expect(action.topLevel).toBe(true); - expectForwardedCfg(invoke, cfg); - expectNoForwardedToolContext(invoke); - }); - - it("maps upload-file to the internal uploadFile action", async () => { - const invoke = createInvokeSpy(); - const cfg = slackConfig(); - - await handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "upload-file", - cfg, - params: { - to: "user:U1", - filePath: "/tmp/report.png", - initialComment: "fresh build", - filename: "build.png", - title: "Build Screenshot", - threadId: "111.222", - }, - } as never, - invoke: invoke as never, - }); - - const action = firstAction(invoke); - expect(action.action).toBe("uploadFile"); - expect(action.to).toBe("user:U1"); - expect(action.filePath).toBe("/tmp/report.png"); - expect(action.initialComment).toBe("fresh build"); - expect(action.filename).toBe("build.png"); - expect(action.title).toBe("Build Screenshot"); - expect(action.threadTs).toBe("111.222"); + expect(firstAction(invoke)).toMatchObject({ action: "uploadFile", ...expected }); expectForwardedCfg(invoke, cfg); expectNoForwardedToolContext(invoke); }); @@ -777,62 +770,6 @@ describe("handleSlackMessageAction", () => { ).rejects.toThrow(/replyBroadcast is only supported for text or block thread replies/i); }); - it("maps upload-file aliases to upload params", async () => { - const invoke = createInvokeSpy(); - const cfg = slackConfig(); - - await handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "upload-file", - cfg, - params: { - channelId: "C1", - media: "/tmp/chart.png", - message: "chart attached", - replyTo: "333.444", - }, - } as never, - invoke: invoke as never, - }); - - const action = firstAction(invoke); - expect(action.action).toBe("uploadFile"); - expect(action.to).toBe("C1"); - expect(action.filePath).toBe("/tmp/chart.png"); - expect(action.initialComment).toBe("chart attached"); - expect(action.threadTs).toBe("333.444"); - expectForwardedCfg(invoke, cfg); - expectNoForwardedToolContext(invoke); - }); - - it("maps upload-file path alias to filePath", async () => { - const invoke = createInvokeSpy(); - const cfg = slackConfig(); - - await handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "upload-file", - cfg, - params: { - to: "channel:C1", - path: "/tmp/report.txt", - initialComment: "path alias", - }, - } as never, - invoke: invoke as never, - }); - - const action = firstAction(invoke); - expect(action.action).toBe("uploadFile"); - expect(action.to).toBe("channel:C1"); - expect(action.filePath).toBe("/tmp/report.txt"); - expect(action.initialComment).toBe("path alias"); - expectForwardedCfg(invoke, cfg); - expectNoForwardedToolContext(invoke); - }); - it("forwards messageId for read actions", async () => { const invoke = createInvokeSpy(); @@ -892,29 +829,26 @@ describe("handleSlackMessageAction", () => { ).rejects.toThrow(/upload-file requires filePath, path, or media/i); }); - it("maps download-file to the internal downloadFile action", async () => { + it.each([ + { + name: "maps download-file to the internal downloadFile action", + params: { channelId: "C1", fileId: "F123", threadId: "111.222" }, + expected: { fileId: "F123", channelId: "C1", threadId: "111.222" }, + }, + { + name: "maps download-file target aliases to scope fields", + params: { to: "channel:C2", fileId: "F999", replyTo: "333.444" }, + expected: { fileId: "F999", channelId: "channel:C2", threadId: "333.444" }, + }, + ])("$name", async ({ params, expected }) => { const invoke = createInvokeSpy(); const cfg = slackConfig(); - await handleSlackMessageAction({ providerId: "slack", - ctx: { - action: "download-file", - cfg, - params: { - channelId: "C1", - fileId: "F123", - threadId: "111.222", - }, - } as never, + ctx: { action: "download-file", cfg, params } as never, invoke: invoke as never, }); - - const action = firstAction(invoke); - expect(action.action).toBe("downloadFile"); - expect(action.fileId).toBe("F123"); - expect(action.channelId).toBe("C1"); - expect(action.threadId).toBe("111.222"); + expect(firstAction(invoke)).toMatchObject({ action: "downloadFile", ...expected }); expectForwardedCfg(invoke, cfg); }); @@ -944,80 +878,30 @@ describe("handleSlackMessageAction", () => { expect(firstInvokeCall(invoke)[2]).toBe(toolContext); }); - it("maps download-file target aliases to scope fields", async () => { - const invoke = createInvokeSpy(); - const cfg = slackConfig(); - - await handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "download-file", - cfg, - params: { - to: "channel:C2", - fileId: "F999", - replyTo: "333.444", - }, - } as never, - invoke: invoke as never, - }); - - const action = firstAction(invoke); - expect(action.action).toBe("downloadFile"); - expect(action.fileId).toBe("F999"); - expect(action.channelId).toBe("channel:C2"); - expect(action.threadId).toBe("333.444"); - expectForwardedCfg(invoke, cfg); - }); - - it("explains that download-file requires fileId, not messageId", async () => { + it.each([ + { + name: "explains that download-file requires fileId, not messageId", + params: { channelId: "C1", messageId: "1777423717.666499" }, + expectedError: /Did you mean to pass fileId/i, + }, + { + name: "explains that download-file requires fileId for message_id aliases", + params: { channelId: "C1", message_id: "1777423717.666499" }, + expectedError: /Did you mean to pass fileId/i, + }, + { + name: "keeps the generic fileId requirement when no message id was supplied", + params: { channelId: "C1" }, + expectedError: /fileId/i, + }, + ])("$name", async ({ params, expectedError }) => { await expect( handleSlackMessageAction({ providerId: "slack", - ctx: { - action: "download-file", - cfg: {}, - params: { - channelId: "C1", - messageId: "1777423717.666499", - }, - } as never, + ctx: { action: "download-file", cfg: {}, params } as never, invoke: createInvokeSpy() as never, }), - ).rejects.toThrow(/Did you mean to pass fileId/i); - }); - - it("explains that download-file requires fileId for message_id aliases", async () => { - await expect( - handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "download-file", - cfg: {}, - params: { - channelId: "C1", - message_id: "1777423717.666499", - }, - } as never, - invoke: createInvokeSpy() as never, - }), - ).rejects.toThrow(/Did you mean to pass fileId/i); - }); - - it("keeps the generic fileId requirement when no message id was supplied", async () => { - await expect( - handleSlackMessageAction({ - providerId: "slack", - ctx: { - action: "download-file", - cfg: {}, - params: { - channelId: "C1", - }, - } as never, - invoke: createInvokeSpy() as never, - }), - ).rejects.toThrow(/fileId/i); + ).rejects.toThrow(expectedError); }); it("defaults member-info userId to the inbound sender when omitted", async () => { diff --git a/extensions/slack/src/monitor/media.test.ts b/extensions/slack/src/monitor/media.test.ts index a54a492ab3a1..ca0740deb546 100644 --- a/extensions/slack/src/monitor/media.test.ts +++ b/extensions/slack/src/monitor/media.test.ts @@ -623,39 +623,25 @@ describe("resolveSlackMedia", () => { expectFetchCalledWithUrl(mockFetch, "https://files.slack.com/fresh.jpg"); }); - it("skips id-only files when files.info returns no private URL", async () => { + it.each([ + { name: "skips id-only files when files.info returns no private URL", fails: false }, + { name: "skips id-only files when files.info fails", fails: true }, + ])("$name", async ({ fails }) => { + const info = vi.fn(); + if (fails) { + info.mockRejectedValue(new Error("files.info failed")); + } else { + info.mockResolvedValue({ file: { id: "F123" } }); + } const mockClient = { - files: { - info: vi.fn().mockResolvedValue({ file: { id: "F123" } }), - }, + files: { info }, } as unknown as WebClient & { files: { info: ReturnType } }; - const result = await resolveSlackMedia({ files: [{ id: "F123", name: "test.jpg" }], client: mockClient, token: "xoxb-test-token", maxBytes: 1024 * 1024, }); - - expect(result).toBeNull(); - expect(mockClient.files.info).toHaveBeenCalledWith({ file: "F123" }); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it("skips id-only files when files.info fails", async () => { - const mockClient = { - files: { - info: vi.fn().mockRejectedValue(new Error("files.info failed")), - }, - } as unknown as WebClient & { files: { info: ReturnType } }; - - const result = await resolveSlackMedia({ - files: [{ id: "F123", name: "test.jpg" }], - client: mockClient, - token: "xoxb-test-token", - maxBytes: 1024 * 1024, - }); - expect(result).toBeNull(); expect(mockClient.files.info).toHaveBeenCalledWith({ file: "F123" }); expect(mockFetch).not.toHaveBeenCalled();