mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 08:40:44 +00:00
fix(slack): surface silent errors in thread starter/history fetch
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).
This commit is contained in:
committed by
Peter Steinberger
parent
e7d33b4870
commit
31e5cd6376
@@ -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<typeof vi.fn<FetchMock>>;
|
||||
@@ -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<typeof resolveSlackThreadStarter>[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<typeof resolveSlackThreadStarter>[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<typeof resolveSlackThreadStarter>[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<typeof resolveSlackThreadStarter>[0]["client"];
|
||||
|
||||
const result = await resolveSlackThreadStarter({
|
||||
channelId: "C1",
|
||||
threadTs: "1.000",
|
||||
client,
|
||||
});
|
||||
|
||||
expect(result).toBeNull();
|
||||
expect(vi.mocked(logVerbose)).toHaveBeenCalledWith(expect.stringContaining("rate_limited"));
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 [];
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user