From 31e5cd6376eec50cc48506eac9aecc288546c60c Mon Sep 17 00:00:00 2001 From: Martin Garramon Date: Sat, 18 Apr 2026 11:15:15 -0300 Subject: [PATCH] fix(slack): surface silent errors in thread starter/history fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #62571. `resolveSlackThreadStarter` and `resolveSlackThreadHistory` in `extensions/slack/src/monitor/media.ts` swallowed ALL errors with bare `catch {}` blocks — auth failures, rate-limit rejections, scope errors, and network blips all mapped to the same silent `null` / `[]` fallback. Operators had no way to distinguish "genuinely empty thread" from "Slack rejected our call". Replaces both bare catches with `logVerbose` calls that include the channel, thread ts, and error message. Behavior is preserved — callers still receive `null` / `[]` — but the failure reason now shows up in verbose logs, matching the pattern already used elsewhere in the Slack extension (see `monitor/context.ts:285`, `send.ts:140`, `actions.ts:49`). Testing: - New `describe("resolveSlackThreadStarter", ...)` block with 4 tests (previously uncovered): success path, empty-text skip, Error throw surfaces via logVerbose with channel/ts/reason, non-Error throw value surfaces via String(err). - Existing `resolveSlackThreadHistory` throws test upgraded to assert the logVerbose call with channel/ts/reason. - `pnpm vitest run extensions/slack/src/monitor/media.test.ts` → 35 passed (31 previous + 4 new). --- extensions/slack/src/monitor/media.test.ts | 95 +++++++++++++++++++++- extensions/slack/src/monitor/media.ts | 11 ++- 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/extensions/slack/src/monitor/media.test.ts b/extensions/slack/src/monitor/media.test.ts index 21d00c1550b..45c154a22cc 100644 --- a/extensions/slack/src/monitor/media.test.ts +++ b/extensions/slack/src/monitor/media.test.ts @@ -2,6 +2,7 @@ import * as ssrf from "openclaw/plugin-sdk/infra-runtime"; import * as mediaFetch from "openclaw/plugin-sdk/media-runtime"; import type { SavedMedia } from "openclaw/plugin-sdk/media-runtime"; import * as mediaStore from "openclaw/plugin-sdk/media-runtime"; +import { logVerbose } from "openclaw/plugin-sdk/runtime-env"; import { type FetchMock, withFetchPreconnect } from "openclaw/plugin-sdk/testing"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { mockPinnedHostnameResolution } from "../../../../src/test-helpers/ssrf.js"; @@ -10,8 +11,16 @@ import { resolveSlackAttachmentContent, resolveSlackMedia, resolveSlackThreadHistory, + resolveSlackThreadStarter, + resetSlackThreadStarterCacheForTest, } from "./media.js"; +vi.mock("openclaw/plugin-sdk/runtime-env", () => ({ + logVerbose: vi.fn(), + danger: (message: string) => message, + shouldLogVerbose: () => false, +})); + // Store original fetch const originalFetch = globalThis.fetch; let mockFetch: ReturnType>; @@ -843,7 +852,8 @@ describe("resolveSlackThreadHistory", () => { expect(replies).not.toHaveBeenCalled(); }); - it("returns empty when Slack API throws", async () => { + it("returns empty and surfaces the error via logVerbose when Slack API throws", async () => { + vi.mocked(logVerbose).mockClear(); const replies = vi.fn().mockRejectedValueOnce(new Error("slack down")); const client = { conversations: { replies }, @@ -857,5 +867,88 @@ describe("resolveSlackThreadHistory", () => { }); expect(result).toEqual([]); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith( + expect.stringContaining("slack thread history fetch failed"), + ); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("slack down")); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("channel=C1")); + }); +}); + +describe("resolveSlackThreadStarter", () => { + beforeEach(() => { + resetSlackThreadStarterCacheForTest(); + vi.mocked(logVerbose).mockClear(); + }); + + it("returns the starter message when the Slack API succeeds", async () => { + const replies = vi.fn().mockResolvedValueOnce({ + messages: [{ text: "hello thread", user: "U1", ts: "1.000" }], + }); + const client = { + conversations: { replies }, + } as unknown as Parameters[0]["client"]; + + const result = await resolveSlackThreadStarter({ + channelId: "C1", + threadTs: "1.000", + client, + }); + + expect(result).toEqual({ text: "hello thread", userId: "U1", botId: undefined, ts: "1.000", files: undefined }); + expect(vi.mocked(logVerbose)).not.toHaveBeenCalled(); + }); + + it("returns null when the starter message has no text and no files", async () => { + const replies = vi.fn().mockResolvedValueOnce({ messages: [{ text: " ", user: "U1" }] }); + const client = { + conversations: { replies }, + } as unknown as Parameters[0]["client"]; + + const result = await resolveSlackThreadStarter({ + channelId: "C1", + threadTs: "1.000", + client, + }); + + expect(result).toBeNull(); + expect(vi.mocked(logVerbose)).not.toHaveBeenCalled(); + }); + + it("returns null and surfaces the error via logVerbose when Slack API throws", async () => { + const replies = vi.fn().mockRejectedValueOnce(new Error("not_in_channel")); + const client = { + conversations: { replies }, + } as unknown as Parameters[0]["client"]; + + const result = await resolveSlackThreadStarter({ + channelId: "C42", + threadTs: "9.999", + client, + }); + + expect(result).toBeNull(); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith( + expect.stringContaining("slack thread starter fetch failed"), + ); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("not_in_channel")); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("channel=C42")); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("ts=9.999")); + }); + + it("surfaces non-Error thrown values as String(err) via logVerbose", async () => { + const replies = vi.fn().mockRejectedValueOnce("rate_limited"); + const client = { + conversations: { replies }, + } as unknown as Parameters[0]["client"]; + + const result = await resolveSlackThreadStarter({ + channelId: "C1", + threadTs: "1.000", + client, + }); + + expect(result).toBeNull(); + expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("rate_limited")); }); }); diff --git a/extensions/slack/src/monitor/media.ts b/extensions/slack/src/monitor/media.ts index 16e47f4526b..d9a59996cac 100644 --- a/extensions/slack/src/monitor/media.ts +++ b/extensions/slack/src/monitor/media.ts @@ -5,6 +5,7 @@ import type { FetchLike } from "openclaw/plugin-sdk/media-runtime"; import { fetchRemoteMedia } from "openclaw/plugin-sdk/media-runtime"; import { saveMediaBuffer } from "openclaw/plugin-sdk/media-runtime"; import { resolveRequestUrl } from "openclaw/plugin-sdk/request-url"; +import { logVerbose } from "openclaw/plugin-sdk/runtime-env"; import { normalizeLowercaseStringOrEmpty, normalizeOptionalLowercaseString, @@ -447,7 +448,10 @@ export async function resolveSlackThreadStarter(params: { }); evictThreadStarterCache(); return starter; - } catch { + } catch (err) { + logVerbose( + `slack thread starter fetch failed channel=${params.channelId} ts=${params.threadTs}: ${err instanceof Error ? err.message : String(err)}`, + ); return null; } } @@ -539,7 +543,10 @@ export async function resolveSlackThreadHistory(params: { ts: msg.ts, files: msg.files, })); - } catch { + } catch (err) { + logVerbose( + `slack thread history fetch failed channel=${params.channelId} ts=${params.threadTs}: ${err instanceof Error ? err.message : String(err)}`, + ); return []; } }