From 3832074cd982ff5fc931970663aa1aceea2afd7d Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 3 Aug 2026 03:21:55 -0700 Subject: [PATCH] fix(mattermost): preserve channel lookup failures (#118622) Co-authored-by: Peter Steinberger --- .../mattermost/src/mattermost/client.ts | 14 +++ .../mattermost/src/mattermost/send.test.ts | 116 +++++++++++++++++- extensions/mattermost/src/mattermost/send.ts | 7 +- .../src/mattermost/target-resolution.test.ts | 4 +- .../src/mattermost/target-resolution.ts | 14 +-- 5 files changed, 138 insertions(+), 17 deletions(-) diff --git a/extensions/mattermost/src/mattermost/client.ts b/extensions/mattermost/src/mattermost/client.ts index 63358cf832b3..71ab707e044b 100644 --- a/extensions/mattermost/src/mattermost/client.ts +++ b/extensions/mattermost/src/mattermost/client.ts @@ -92,6 +92,20 @@ type MattermostFileInfo = { size?: number | null; }; +export function parseMattermostApiStatus(error: unknown): number | undefined { + if (!error || typeof error !== "object") { + return undefined; + } + const message = "message" in error && typeof error.message === "string" ? error.message : ""; + // Read only the provider's status prefix; upstream details can mention other HTTP statuses. + const match = /Mattermost API (\d{3})\b/.exec(message); + if (!match) { + return undefined; + } + const status = Number(match[1]); + return Number.isFinite(status) ? status : undefined; +} + export function normalizeMattermostBaseUrl(raw?: string | null): string | undefined { const trimmed = raw?.trim(); if (!trimmed) { diff --git a/extensions/mattermost/src/mattermost/send.test.ts b/extensions/mattermost/src/mattermost/send.test.ts index 302c69bd561e..3f2d8fc6bc7d 100644 --- a/extensions/mattermost/src/mattermost/send.test.ts +++ b/extensions/mattermost/src/mattermost/send.test.ts @@ -110,6 +110,34 @@ function directChannelRetryCall() { ) as [unknown, unknown, MattermostDirectRetryOptions?]; } +async function createMattermostProviderFailure( + status: number, + statusText: string, + message: string, +): Promise { + const { createMattermostClient } = + await vi.importActual("./client.js"); + const client = createMattermostClient({ + baseUrl: "https://mattermost.example.com", + botToken: "test-bot-token", + fetchImpl: async () => + new Response(JSON.stringify({ message }), { + status, + statusText, + headers: { "content-type": "application/json" }, + }), + }); + try { + await client.request("/teams/team-first/channels/name/release-alerts"); + } catch (error) { + if (error instanceof Error) { + return error; + } + throw error; + } + throw new Error("Expected the Mattermost provider request to fail"); +} + vi.mock("../../runtime-api.js", () => ({ loadOutboundMediaFromUrl: mockState.loadOutboundMediaFromUrl, })); @@ -162,7 +190,9 @@ vi.mock("./accounts.js", () => ({ resolveMattermostAccount: mockState.resolveMattermostAccount, })); -vi.mock("./client.js", () => ({ +vi.mock("./client.js", async () => ({ + parseMattermostApiStatus: (await vi.importActual("./client.js")) + .parseMattermostApiStatus, createMattermostClient: mockState.createMattermostClient, createMattermostDirectChannelWithRetry: mockState.createMattermostDirectChannelWithRetry, createMattermostPost: mockState.createMattermostPost, @@ -263,6 +293,90 @@ describe("sendMessageMattermost", () => { }); }); + it("continues searching later teams only when a channel is genuinely absent", async () => { + mockState.fetchMattermostUserTeams.mockResolvedValueOnce([ + { id: "team-first" }, + { id: "team-second" }, + ]); + mockState.fetchMattermostChannelByName + .mockRejectedValueOnce(await createMattermostProviderFailure(404, "Not Found", "missing")) + .mockResolvedValueOnce({ id: "channel-second" }); + + const result = await sendMessageMattermost("#release-alerts", "hello", { cfg: TEST_CFG }); + + expect(result.channelId).toBe("channel-second"); + expect(mockState.fetchMattermostChannelByName).toHaveBeenNthCalledWith( + 1, + {}, + "team-first", + "release-alerts", + ); + expect(mockState.fetchMattermostChannelByName).toHaveBeenNthCalledWith( + 2, + {}, + "team-second", + "release-alerts", + ); + expect(mockState.createMattermostPost).toHaveBeenCalledOnce(); + }); + + it("reports a missing named channel after every team returns not found", async () => { + mockState.fetchMattermostUserTeams.mockResolvedValueOnce([ + { id: "team-first" }, + { id: "team-second" }, + ]); + mockState.fetchMattermostChannelByName.mockRejectedValue( + await createMattermostProviderFailure(404, "Not Found", "missing channel"), + ); + + await expect( + sendMessageMattermost("#release-alerts", "hello", { cfg: TEST_CFG }), + ).rejects.toThrow('Mattermost channel "#release-alerts" not found in any team'); + + expect(mockState.fetchMattermostChannelByName).toHaveBeenCalledTimes(2); + expect(mockState.createMattermostPost).not.toHaveBeenCalled(); + }); + + it.each([ + { + name: "an expired bot token", + createError: () => createMattermostProviderFailure(401, "Unauthorized", "bot token expired"), + }, + { + name: "missing channel permissions", + createError: () => createMattermostProviderFailure(403, "Forbidden", "access denied"), + }, + { + name: "provider rate limiting", + createError: () => createMattermostProviderFailure(429, "Too Many Requests", "retry later"), + }, + { + name: "an outage whose detail mentions a missing resource", + createError: () => + createMattermostProviderFailure(503, "Service Unavailable", "upstream returned 404"), + }, + { + name: "a network failure", + createError: async () => new Error("connect ECONNRESET 192.0.2.12:443"), + }, + ])("preserves $name while resolving a named channel", async ({ createError }) => { + const error = await createError(); + mockState.fetchMattermostUserTeams.mockResolvedValueOnce([ + { id: "team-first" }, + { id: "team-second" }, + ]); + mockState.fetchMattermostChannelByName + .mockRejectedValueOnce(error) + .mockResolvedValueOnce({ id: "channel-second" }); + + await expect(sendMessageMattermost("#release-alerts", "hello", { cfg: TEST_CFG })).rejects.toBe( + error, + ); + + expect(mockState.fetchMattermostChannelByName).toHaveBeenCalledOnce(); + expect(mockState.createMattermostPost).not.toHaveBeenCalled(); + }); + it.each(MATTERMOST_MARKDOWN_GOLDENS)("$name", async ({ input, before, after }) => { expect(convertMarkdownTables(input, "code")).toBe(before); diff --git a/extensions/mattermost/src/mattermost/send.ts b/extensions/mattermost/src/mattermost/send.ts index 31f289c6db4d..880ee1f17487 100644 --- a/extensions/mattermost/src/mattermost/send.ts +++ b/extensions/mattermost/src/mattermost/send.ts @@ -26,6 +26,7 @@ import { fetchMattermostUserByUsername, fetchMattermostUserTeams, normalizeMattermostBaseUrl, + parseMattermostApiStatus, uploadMattermostFile, type MattermostUser, type CreateDmChannelRetryOptions, @@ -233,8 +234,10 @@ async function resolveChannelIdByName(params: { ); return channel.id; } - } catch { - // Channel not found in this team, try next + } catch (error) { + if (parseMattermostApiStatus(error) !== 404) { + throw error; + } } } throw new Error(`Mattermost channel "#${name}" not found in any team the bot belongs to`); diff --git a/extensions/mattermost/src/mattermost/target-resolution.test.ts b/extensions/mattermost/src/mattermost/target-resolution.test.ts index fc550f92b5bf..73a751dff814 100644 --- a/extensions/mattermost/src/mattermost/target-resolution.test.ts +++ b/extensions/mattermost/src/mattermost/target-resolution.test.ts @@ -11,7 +11,9 @@ vi.mock("./accounts.js", () => ({ resolveMattermostAccount, })); -vi.mock("./client.js", () => ({ +vi.mock("./client.js", async () => ({ + parseMattermostApiStatus: (await vi.importActual("./client.js")) + .parseMattermostApiStatus, createMattermostClient, fetchMattermostUser, fetchMattermostChannel, diff --git a/extensions/mattermost/src/mattermost/target-resolution.ts b/extensions/mattermost/src/mattermost/target-resolution.ts index 53d70bf76a2f..23b1324533c0 100644 --- a/extensions/mattermost/src/mattermost/target-resolution.ts +++ b/extensions/mattermost/src/mattermost/target-resolution.ts @@ -11,6 +11,7 @@ import { fetchMattermostChannel, fetchMattermostUser, normalizeMattermostBaseUrl, + parseMattermostApiStatus, } from "./client.js"; import { resolveMattermostTrustedChatKind } from "./monitor-auth.js"; import type { OpenClawConfig } from "./runtime-api.js"; @@ -138,19 +139,6 @@ function isExplicitMattermostTarget(raw: string): boolean { ); } -function parseMattermostApiStatus(err: unknown): number | undefined { - if (!err || typeof err !== "object") { - return undefined; - } - const msg = "message" in err && typeof err.message === "string" ? err.message : ""; - const match = /Mattermost API (\d{3})\b/.exec(msg); - if (!match) { - return undefined; - } - const code = Number(match[1]); - return Number.isFinite(code) ? code : undefined; -} - export async function resolveMattermostOpaqueTarget(params: { input: string; cfg?: OpenClawConfig;