fix(slack): surface silent errors in thread starter/history fetch#68594
Conversation
Greptile SummaryReplaces two bare Confidence Score: 5/5Safe to merge — minimal targeted change with no behavior impact for callers. Both changes are one-liner catch-block upgrades that preserve existing fallback return values. The logVerbose import and call site follow the established Slack extension pattern. Tests cover success, empty-text skip, Error throw, non-Error throw, and the channel/ts/reason content of the log message. No P0 or P1 findings. No files require special attention. Reviews (1): Last reviewed commit: "fix(slack): surface silent errors in thr..." | Re-trigger Greptile |
a2ce7ea to
f9d6d22
Compare
Fixes openclaw#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).
The `lint:tmp:no-raw-channel-fetch` allowlist pins exact line numbers
(scripts/check-no-raw-channel-fetch.mjs:63-65). The previous commit
added `import { logVerbose } from "openclaw/plugin-sdk/runtime-env";`
on line 8 of `extensions/slack/src/monitor/media.ts`, shifting the
three allowlisted raw `fetch()` callsites from 96/115/120 → 97/116/121.
Updates the allowlist to match the new positions. No behavior change —
the same callsites remain allowlisted.
f9d6d22 to
8b776ad
Compare
|
Landed via rebase. Thanks @martingarramon. Main commits:
Validation:
|
Adds 5 vitest cases for postSlackMessageBestEffort's silent retry behavior when Slack rejects a chat:write.customize-identity post: - Retry on err.data.needed matching chat:write.customize - Retry on chat:write.customize in response_metadata.acceptedScopes - Retry on chat:write.customize in response_metadata.scopes - Rethrow on different missing_scope (e.g. channels:history) - Rethrow when identity is empty (hasCustomIdentity returns false) Verifies call count + logVerbose emission in each case. Follow-up to openclaw#68594. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Fixes #62571.
Problem
resolveSlackThreadStarterandresolveSlackThreadHistoryinextensions/slack/src/monitor/media.tsswallow ALL errors via barecatch {}blocks at lines 450–452 and 542–544 respectively. Auth failures, rate-limit rejections, scope errors (not_in_channel,channel_not_found), and network blips all map to the same silentnull/[]fallback. Operators have no way to distinguish "genuinely empty thread" from "Slack rejected our call", and thread-context hydration can degrade silently in production.Why it matters
Thread starter/history hydration is called at the front of every new Slack thread session to populate context. When it silently returns
null/[], the agent proceeds without the missing context and the operator sees no log line explaining why. The fix preserves that best-effort behavior but surfaces the reason.What changed
extensions/slack/src/monitor/media.ts— importslogVerbosefromopenclaw/plugin-sdk/runtime-envand replaces both barecatch {}blocks withcatch (err) { logVerbose(\... channel=${params.channelId} ts=${params.threadTs}: ${err}`); return null/[]; }. Matches the established pattern already used elsewhere in this extension (monitor/context.ts:285,send.ts:140,actions.ts:49`).extensions/slack/src/monitor/media.test.ts— mockslogVerboseviavi.mock("openclaw/plugin-sdk/runtime-env", ...). Upgrades the existingresolveSlackThreadHistorythrows test to assert the log call with channel/ts/reason. Adds a newdescribe("resolveSlackThreadStarter", ...)block with 4 tests (the function had no direct coverage): success path, empty-text skip,Errorthrow surfaces vialogVerbosewith channel + ts + reason, non-Errorthrown value surfaces viaString(err).No behavior change for callers — they still receive
null/[]on failure. The log is verbose-level, so no noise increase for normal operation.Testing
pnpm vitest run extensions/slack/src/monitor/media.test.ts→ 35 passed (31 previous + 4 new).What did NOT change (scope boundary)
logVerbose(silent unlessOPENCLAW_DEBUGis on), matching sibling slack logging.Root Cause
catch {}blocks added when the resolvers were first written, never updated as the Slack extension grewlogVerbosefor error-surface diagnostics elsewhere.Regression Test Plan
extensions/slack/src/monitor/media.test.tsconversations.repliesthrows,logVerboseis called with channel + ts + reason, and the resolver still returns the fallback.