From 82861968c2185cc3c357660f0f198be1bcc4eb4f Mon Sep 17 00:00:00 2001 From: Echo Date: Mon, 16 Feb 2026 17:34:36 -0500 Subject: [PATCH] fix(mattermost): address review feedback on reactions PR --- extensions/mattermost/src/channel.test.ts | 50 +++++++++++++++++++ extensions/mattermost/src/channel.ts | 27 +++++----- .../src/mattermost/monitor-websocket.ts | 26 ++++++---- .../src/mattermost/reactions.test.ts | 30 +++++++++++ .../mattermost/src/mattermost/reactions.ts | 28 +++++++---- 5 files changed, 128 insertions(+), 33 deletions(-) diff --git a/extensions/mattermost/src/channel.test.ts b/extensions/mattermost/src/channel.test.ts index 016805d0ba..64bd2bd0c3 100644 --- a/extensions/mattermost/src/channel.test.ts +++ b/extensions/mattermost/src/channel.test.ts @@ -85,6 +85,56 @@ describe("mattermostPlugin", () => { expect(actions).not.toContain("send"); }); + it("respects per-account actions.reactions in listActions", () => { + const cfg: OpenClawConfig = { + channels: { + mattermost: { + enabled: true, + actions: { reactions: false }, + accounts: { + default: { + enabled: true, + botToken: "test-token", + baseUrl: "https://chat.example.com", + actions: { reactions: true }, + }, + }, + }, + }, + }; + + const actions = mattermostPlugin.actions?.listActions?.({ cfg }) ?? []; + expect(actions).toContain("react"); + }); + + it("blocks react when default account disables reactions and accountId is omitted", async () => { + const cfg: OpenClawConfig = { + channels: { + mattermost: { + enabled: true, + actions: { reactions: true }, + accounts: { + default: { + enabled: true, + botToken: "test-token", + baseUrl: "https://chat.example.com", + actions: { reactions: false }, + }, + }, + }, + }, + }; + + await expect( + mattermostPlugin.actions?.handleAction?.({ + channel: "mattermost", + action: "react", + params: { messageId: "POST1", emoji: "thumbsup" }, + cfg, + } as any), + ).rejects.toThrow("Mattermost reactions are disabled in config"); + }); + it("handles react by calling Mattermost reactions API", async () => { const cfg: OpenClawConfig = { channels: { diff --git a/extensions/mattermost/src/channel.ts b/extensions/mattermost/src/channel.ts index 144c963fe9..43ee15fef1 100644 --- a/extensions/mattermost/src/channel.ts +++ b/extensions/mattermost/src/channel.ts @@ -30,22 +30,22 @@ import { getMattermostRuntime } from "./runtime.js"; const mattermostMessageActions: ChannelMessageActionAdapter = { listActions: ({ cfg }) => { - const accounts = listMattermostAccountIds(cfg) + const actionsConfig = cfg.channels?.mattermost?.actions as { reactions?: boolean } | undefined; + const baseReactions = actionsConfig?.reactions; + const hasReactionCapableAccount = listMattermostAccountIds(cfg) .map((accountId) => resolveMattermostAccount({ cfg, accountId })) .filter((account) => account.enabled) - .filter((account) => Boolean(account.botToken?.trim() && account.baseUrl?.trim())); + .filter((account) => Boolean(account.botToken?.trim() && account.baseUrl?.trim())) + .some((account) => { + const accountActions = account.config.actions as { reactions?: boolean } | undefined; + return (accountActions?.reactions ?? baseReactions ?? true) !== false; + }); - if (accounts.length === 0) { + if (!hasReactionCapableAccount) { return []; } - const actions: ChannelMessageActionName[] = []; - const actionsConfig = cfg.channels?.mattermost?.actions as { reactions?: boolean } | undefined; - const reactionsEnabled = actionsConfig?.reactions !== false; - if (reactionsEnabled) { - actions.push("react"); - } - return actions; + return ["react"]; }, supportsAction: ({ action }) => { return action === "react"; @@ -57,7 +57,8 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { // Check reactions gate: per-account config takes precedence over base config const mmBase = cfg?.channels?.mattermost as Record | undefined; const accounts = mmBase?.accounts as Record> | undefined; - const acctConfig = accountId && accounts ? accounts[accountId] : undefined; + const resolvedAccountId = accountId ?? resolveDefaultMattermostAccountId(cfg); + const acctConfig = accounts?.[resolvedAccountId]; const acctActions = acctConfig?.actions as { reactions?: boolean } | undefined; const baseActions = mmBase?.actions as { reactions?: boolean } | undefined; const reactionsEnabled = acctActions?.reactions ?? baseActions?.reactions ?? true; @@ -88,7 +89,7 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { cfg, postId, emojiName, - accountId: accountId ?? undefined, + accountId: resolvedAccountId, }); if (!result.ok) { throw new Error(result.error); @@ -105,7 +106,7 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { cfg, postId, emojiName, - accountId: accountId ?? undefined, + accountId: resolvedAccountId, }); if (!result.ok) { throw new Error(result.error); diff --git a/extensions/mattermost/src/mattermost/monitor-websocket.ts b/extensions/mattermost/src/mattermost/monitor-websocket.ts index 1595aa5162..19494c1a01 100644 --- a/extensions/mattermost/src/mattermost/monitor-websocket.ts +++ b/extensions/mattermost/src/mattermost/monitor-websocket.ts @@ -59,16 +59,9 @@ type CreateMattermostConnectOnceOpts = { export const defaultMattermostWebSocketFactory: MattermostWebSocketFactory = (url) => new WebSocket(url) as MattermostWebSocketLike; -export function parsePostedEvent( - data: WebSocket.RawData, +export function parsePostedPayload( + payload: MattermostEventPayload, ): { payload: MattermostEventPayload; post: MattermostPost } | null { - const raw = rawDataToString(data); - let payload: MattermostEventPayload; - try { - payload = JSON.parse(raw) as MattermostEventPayload; - } catch { - return null; - } if (payload.event !== "posted") { return null; } @@ -92,6 +85,19 @@ export function parsePostedEvent( return { payload, post }; } +export function parsePostedEvent( + data: WebSocket.RawData, +): { payload: MattermostEventPayload; post: MattermostPost } | null { + const raw = rawDataToString(data); + let payload: MattermostEventPayload; + try { + payload = JSON.parse(raw) as MattermostEventPayload; + } catch { + return null; + } + return parsePostedPayload(payload); +} + export function createMattermostConnectOnce( opts: CreateMattermostConnectOnceOpts, ): () => Promise { @@ -160,7 +166,7 @@ export function createMattermostConnectOnce( if (payload.event !== "posted") { return; } - const parsed = parsePostedEvent(data); + const parsed = parsePostedPayload(payload); if (!parsed) { return; } diff --git a/extensions/mattermost/src/mattermost/reactions.test.ts b/extensions/mattermost/src/mattermost/reactions.test.ts index dd8fe72657..c51fa6d627 100644 --- a/extensions/mattermost/src/mattermost/reactions.test.ts +++ b/extensions/mattermost/src/mattermost/reactions.test.ts @@ -49,6 +49,36 @@ describe("mattermost reactions", () => { expect(fetchImpl).toHaveBeenCalled(); }); + it("returns a Result error when add reaction API call fails", async () => { + const fetchImpl = vi.fn(async (url: any) => { + if (String(url).endsWith("/api/v4/users/me")) { + return new Response(JSON.stringify({ id: "BOT123" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + } + if (String(url).endsWith("/api/v4/reactions")) { + return new Response(JSON.stringify({ id: "err", message: "boom" }), { + status: 500, + headers: { "content-type": "application/json" }, + }); + } + throw new Error(`unexpected url: ${url}`); + }); + + const result = await addMattermostReaction({ + cfg: createCfg(), + postId: "POST1", + emojiName: "thumbsup", + fetchImpl, + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("Mattermost add reaction failed"); + } + }); + it("removes reactions by calling /users/me then DELETE /users/:id/posts/:postId/reactions/:emoji", async () => { const fetchImpl = vi.fn(async (url: any, init?: any) => { if (String(url).endsWith("/api/v4/users/me")) { diff --git a/extensions/mattermost/src/mattermost/reactions.ts b/extensions/mattermost/src/mattermost/reactions.ts index 1c557a263e..03a01c03f2 100644 --- a/extensions/mattermost/src/mattermost/reactions.ts +++ b/extensions/mattermost/src/mattermost/reactions.ts @@ -50,11 +50,15 @@ export async function addMattermostReaction(params: { return { ok: false, error: "Mattermost reactions failed: could not resolve bot user id." }; } - await createReaction(client, { - userId, - postId: params.postId, - emojiName: params.emojiName, - }); + try { + await createReaction(client, { + userId, + postId: params.postId, + emojiName: params.emojiName, + }); + } catch (err) { + return { ok: false, error: `Mattermost add reaction failed: ${String(err)}` }; + } return { ok: true }; } @@ -85,11 +89,15 @@ export async function removeMattermostReaction(params: { return { ok: false, error: "Mattermost reactions failed: could not resolve bot user id." }; } - await deleteReaction(client, { - userId, - postId: params.postId, - emojiName: params.emojiName, - }); + try { + await deleteReaction(client, { + userId, + postId: params.postId, + emojiName: params.emojiName, + }); + } catch (err) { + return { ok: false, error: `Mattermost remove reaction failed: ${String(err)}` }; + } return { ok: true }; }