From fe0a6db2b47c5cd4c528d4bd6b33736a3e2aa4f9 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 12 Jul 2026 17:50:14 -0700 Subject: [PATCH] fix(slack): warn on disabled channel drops (#105790) --- docs/concepts/qa-e2e-automation.md | 2 + .../slack/slack-live.runtime.test.ts | 59 ++++++++++ .../slack/slack-live.runtime.ts | 70 +++++++++++- extensions/slack/src/monitor/context.ts | 54 ++++++--- extensions/slack/src/monitor/monitor.test.ts | 107 +++++++++++++++++- 5 files changed, 273 insertions(+), 19 deletions(-) diff --git a/docs/concepts/qa-e2e-automation.md b/docs/concepts/qa-e2e-automation.md index e85b9a33c8e8..387eeaa19953 100644 --- a/docs/concepts/qa-e2e-automation.md +++ b/docs/concepts/qa-e2e-automation.md @@ -612,6 +612,8 @@ Imperative Slack scenarios (`extensions/qa-lab/src/live-transports/slack/slack-l - `slack-canary` - `slack-mention-gating` - `slack-allowlist-block` +- `slack-channel-disabled-warning` - opt-in real-Slack probe that confirms a + configured disabled channel emits a structured warning without replying. - `slack-top-level-reply-shape` - `slack-restart-resume` - `slack-progress-commentary-true`, `slack-progress-commentary-false`, diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts index ae4f9a6d1f22..5f8529199592 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.test.ts @@ -112,9 +112,11 @@ describe("Slack live QA runtime helpers", () => { "slack-approval-plugin-native", "slack-codex-approval-exec-native", "slack-codex-approval-plugin-native", + "slack-channel-disabled-warning", ]) .map((scenario) => scenario.id), ).toEqual([ + "slack-channel-disabled-warning", "slack-progress-commentary-true", "slack-progress-commentary-false", "slack-progress-commentary-omitted", @@ -144,6 +146,9 @@ describe("Slack live QA runtime helpers", () => { expect(testing.findScenario().map((scenario) => scenario.id)).not.toContain( "slack-progress-commentary-true", ); + expect(testing.findScenario().map((scenario) => scenario.id)).not.toContain( + "slack-channel-disabled-warning", + ); expect(testing.findScenario(["slack-codex-approval-exec-native"])[0]?.forcedRuntime).toBe( "codex", ); @@ -269,6 +274,7 @@ describe("Slack live QA runtime helpers", () => { driverBotUserId: "U999999999", overrides: { allowFrom: ["U_NEVER_ALLOWED"], + channelEnabled: false, users: ["U_NEVER_ALLOWED"], }, sutAccountId: "sut", @@ -279,9 +285,62 @@ describe("Slack live QA runtime helpers", () => { const account = cfg.channels?.slack?.accounts?.sut; expect(account?.allowFrom).toEqual(["U_NEVER_ALLOWED"]); + expect(account?.channels?.C123456789?.enabled).toBe(false); expect(account?.channels?.C123456789?.users).toEqual(["U_NEVER_ALLOWED"]); }); + it("configures and verifies the disabled-channel warning scenario", async () => { + const scenario = testing.findScenario(["slack-channel-disabled-warning"])[0]; + expect(scenario?.configOverrides?.channelEnabled).toBe(false); + + const run = scenario?.buildRun("U999999999"); + const beforeRun = run && "beforeRun" in run ? run.beforeRun : undefined; + const afterNoReply = run && "afterNoReply" in run ? run.afterNoReply : undefined; + expect(beforeRun).toBeTypeOf("function"); + expect(afterNoReply).toBeTypeOf("function"); + const call = vi + .fn() + .mockResolvedValueOnce({ cursor: 12 }) + .mockResolvedValueOnce({ + lines: ["Slack channel denied by configuration channel_not_allowed channel_disabled"], + }); + await beforeRun?.({ + gateway: { + call, + }, + } as never); + await expect( + afterNoReply?.({ + gateway: { + call, + }, + } as never), + ).resolves.toBe("structured disabled-channel warning observed"); + expect(call).toHaveBeenNthCalledWith( + 1, + "logs.tail", + { limit: 1, maxBytes: 32_000 }, + { timeoutMs: 20_000 }, + ); + expect(call).toHaveBeenCalledWith( + "logs.tail", + { cursor: 12, limit: 200, maxBytes: 256_000 }, + { timeoutMs: 20_000 }, + ); + await expect( + afterNoReply?.({ + gateway: { + call: vi.fn(async () => ({ + lines: [ + "Slack channel denied by configuration channel_not_allowed", + "channel_disabled", + ], + })), + }, + } as never), + ).rejects.toThrow("did not emit the structured warning"); + }); + it("builds the Slack progress commentary true, false, omitted, and dedupe configs", () => { const buildScenarioConfig = (scenarioId: string) => { const scenario = testing.findScenario([scenarioId])[0]; diff --git a/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.ts b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.ts index 39f8453afdb3..d5b573a85cb4 100644 --- a/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.ts +++ b/extensions/qa-lab/src/live-transports/slack/slack-live.runtime.ts @@ -78,6 +78,7 @@ const SLACK_QA_APPROVAL_CHECKPOINT_DEFAULT_TIMEOUT_MS = 120_000; const SLACK_QA_REACTION_VERIFY_TIMEOUT_MS = 15_000; const SLACK_QA_NATIVE_DATA_VERIFY_TIMEOUT_MS = 15_000; const SLACK_QA_INVALID_TABLE_DATA_ROW_COUNT = 101; +const SLACK_QA_LOG_TAIL_TIMEOUT_MS = 20_000; const SLACK_QA_INVALID_TABLE_CAPTION = "QA invalid_blocks fallback"; const SLACK_QA_INVALID_TABLE_HEADERS = ["Row", "Value"] as const; const SLACK_QA_CHART_TITLE = "QA latency trend"; @@ -139,6 +140,7 @@ type SlackQaScenarioId = | "slack-codex-approval-exec-native" | "slack-codex-approval-plugin-native" | "slack-chart-presentation-native" + | "slack-channel-disabled-warning" | "slack-mention-gating" | "slack-progress-commentary-false" | "slack-progress-commentary-omitted" @@ -178,10 +180,12 @@ function resolveSlackQaSutAccountId(value?: string) { } type SlackQaMessageScenarioRun = { + afterNoReply?: (context: SlackQaScenarioContext) => Promise; kind?: "message"; expectReply: boolean; input: string; matchText: string; + preserveGatewayDebug?: boolean; settleObservedMs?: number; verify?: (message: SlackMessage, context: { requestThreadTs: string; sentTs: string }) => void; verifyObserved?: (params: { @@ -245,6 +249,7 @@ type SlackQaBeforeRunResult = type SlackQaConfigOverrides = { allowFrom?: string[]; + channelEnabled?: boolean; approvals?: { exec?: boolean; plugin?: boolean; @@ -666,6 +671,52 @@ const SLACK_QA_SCENARIOS: SlackQaScenarioDefinition[] = [ }; }, }, + { + id: "slack-channel-disabled-warning", + title: "Slack disabled channel warns and does not trigger", + timeoutMs: 8_000, + defaultEnabled: false, + configOverrides: { channelEnabled: false }, + buildRun: (sutUserId) => { + const marker = `SLACK_QA_DISABLED_${randomUUID().slice(0, 8).toUpperCase()}`; + let logCursor = 0; + return { + expectReply: false, + input: `<@${sutUserId}> reply with only this exact marker: ${marker}`, + matchText: marker, + preserveGatewayDebug: true, + beforeRun: async ({ gateway }) => { + const gatewayLogTail = (await gateway.call( + "logs.tail", + { limit: 1, maxBytes: 32_000 }, + { timeoutMs: SLACK_QA_LOG_TAIL_TIMEOUT_MS }, + )) as { cursor?: unknown }; + logCursor = typeof gatewayLogTail.cursor === "number" ? gatewayLogTail.cursor : 0; + }, + afterNoReply: async ({ gateway }) => { + const gatewayLogTail = (await gateway.call( + "logs.tail", + { cursor: logCursor, limit: 200, maxBytes: 256_000 }, + { timeoutMs: SLACK_QA_LOG_TAIL_TIMEOUT_MS }, + )) as { lines?: unknown }; + const gatewayLogLines = Array.isArray(gatewayLogTail.lines) + ? gatewayLogTail.lines.filter((line): line is string => typeof line === "string") + : []; + const expectedFields = [ + "Slack channel denied by configuration", + "channel_not_allowed", + "channel_disabled", + ]; + if ( + !gatewayLogLines.some((line) => expectedFields.every((field) => line.includes(field))) + ) { + throw new Error("disabled Slack channel did not emit the structured warning"); + } + return "structured disabled-channel warning observed"; + }, + }; + }, + }, { id: "slack-top-level-reply-shape", standardId: "top-level-reply-shape", @@ -1246,7 +1297,7 @@ function buildSlackQaConfig( ...(execApprovalsConfig ? { execApprovals: execApprovalsConfig } : {}), channels: { [params.channelId]: { - enabled: true, + enabled: params.overrides?.channelEnabled ?? true, requireMention: true, allowBots: true, users: params.overrides?.users ?? [params.driverBotUserId], @@ -3486,13 +3537,26 @@ export async function runSlackQaLive(params: { sutIdentity, timeoutMs: scenario.timeoutMs, }); + const afterNoReplyDetails = await scenarioRun.afterNoReply?.({ + ...baseScenarioContext, + sentTs: sent.ts, + }); + if (scenarioRun.preserveGatewayDebug) { + preserveAttemptGatewayDebug = true; + preservedGatewayDebugArtifacts = true; + } scenarioResults.push({ id: scenario.id, title: scenario.title, standardId: scenario.standardId, status: "pass", - details: - scenarioAttempt > 1 ? `no reply; retried ${scenarioAttempt - 1}x` : "no reply", + details: [ + "no reply", + afterNoReplyDetails, + scenarioAttempt > 1 ? `retried ${scenarioAttempt - 1}x` : undefined, + ] + .filter(Boolean) + .join("; "), }); } break; diff --git a/extensions/slack/src/monitor/context.ts b/extensions/slack/src/monitor/context.ts index 8de4d8108f54..9ba2a23f361b 100644 --- a/extensions/slack/src/monitor/context.ts +++ b/extensions/slack/src/monitor/context.ts @@ -64,6 +64,8 @@ type SlackChannelCacheEntry = { const SLACK_ASSISTANT_THREAD_CONTEXT_METADATA_EVENT = "assistant_thread_context"; const SLACK_CHANNEL_CACHE_MAX_ENTRIES = 1024; +const SLACK_CHANNEL_DENIAL_WARNING_TTL_MS = 5 * 60_000; +const SLACK_CHANNEL_DENIAL_WARNING_MAX_ENTRIES = 1024; export function buildSlackAssistantThreadMetadata( context: Omit, @@ -266,6 +268,11 @@ export function createSlackMonitorContext(params: { const channelCache = new Map(); const userCache = new Map(); const seenMessages = createDedupeCache({ ttlMs: 60_000, maxSize: 500 }); + // Rate-limit active denials while retaining periodic evidence; bound keys against config churn. + const channelDenialWarnings = createDedupeCache({ + ttlMs: SLACK_CHANNEL_DENIAL_WARNING_TTL_MS, + maxSize: SLACK_CHANNEL_DENIAL_WARNING_MAX_ENTRIES, + }); const assistantThreadContexts = new Map(); let lastAssistantContextCleanupAt = Date.now(); @@ -660,26 +667,43 @@ export function createSlackMonitorContext(params: { const channelMatchMeta = formatAllowlistMatchMeta(channelConfig); const channelAllowed = channelConfig?.allowed !== false; const channelAllowlistConfigured = hasChannelAllowlistConfig; - if ( - !isSlackChannelAllowedByPolicy({ - groupPolicy: params.groupPolicy, - channelAllowlistConfigured, - channelAllowed, - }) - ) { + const allowedByPolicy = isSlackChannelAllowedByPolicy({ + groupPolicy: params.groupPolicy, + channelAllowlistConfigured, + channelAllowed, + }); + const explicitlyDisabled = + params.groupPolicy !== "disabled" && + channelConfig?.allowed === false && + channelConfig.matchSource !== undefined; + // Open policy still honors an explicit room disable; unlisted rooms remain open. + const shouldDrop = !allowedByPolicy || (params.groupPolicy === "open" && explicitlyDisabled); + if (shouldDrop) { + if (explicitlyDisabled) { + const reason = "channel_not_allowed"; + const warningKey = `${params.accountId}:${p.channelId}:${reason}`; + if (!channelDenialWarnings.peek(warningKey)) { + channelDenialWarnings.check(warningKey); + logger.warn( + { + provider: "slack", + accountId: params.accountId, + channelId: p.channelId, + reason, + cause: "channel_disabled", + groupPolicy: params.groupPolicy, + matchSource: channelConfig.matchSource, + matchKey: channelConfig.matchKey, + }, + "Slack channel denied by configuration", + ); + } + } logVerbose( `slack: drop channel ${p.channelId} (groupPolicy=${params.groupPolicy}, ${channelMatchMeta})`, ); return false; } - // When groupPolicy is "open", only block channels that are EXPLICITLY denied - // (i.e., have a matching config entry with allow:false). Channels not in the - // config (matchSource undefined) should be allowed under open policy. - const hasExplicitConfig = Boolean(channelConfig?.matchSource); - if (!channelAllowed && (params.groupPolicy !== "open" || hasExplicitConfig)) { - logVerbose(`slack: drop channel ${p.channelId} (${channelMatchMeta})`); - return false; - } logVerbose(`slack: allow channel ${p.channelId} (${channelMatchMeta})`); } diff --git a/extensions/slack/src/monitor/monitor.test.ts b/extensions/slack/src/monitor/monitor.test.ts index 5b4b730da824..be1dfddc2407 100644 --- a/extensions/slack/src/monitor/monitor.test.ts +++ b/extensions/slack/src/monitor/monitor.test.ts @@ -2,7 +2,7 @@ import type { App } from "@slack/bolt"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; import type { RuntimeEnv } from "openclaw/plugin-sdk/runtime-env"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { resolveSlackChannelConfig } from "./channel-config.js"; import { createSlackMonitorContext, normalizeSlackChannelType } from "./context.js"; @@ -375,6 +375,111 @@ describe("isChannelAllowed with groupPolicy and channelsConfig", () => { expect(ctx.isChannelAllowed({ channelId: "C_UNLISTED", channelType: "channel" })).toBe(true); }); + it("warns once per explicitly disabled direct channel", () => { + const ctx = createSlackMonitorContext({ + ...baseParams(), + accountId: "work", + groupPolicy: "open", + channelsConfig: { + C_DENIED: { enabled: false }, + C_OTHER: { enabled: false }, + }, + }); + const warnSpy = vi.spyOn(ctx.logger, "warn").mockImplementation(() => undefined); + + expect(ctx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe(false); + expect(ctx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe(false); + expect(ctx.isChannelAllowed({ channelId: "C_OTHER", channelType: "channel" })).toBe(false); + + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy).toHaveBeenNthCalledWith( + 1, + { + provider: "slack", + accountId: "work", + channelId: "C_DENIED", + reason: "channel_not_allowed", + cause: "channel_disabled", + groupPolicy: "open", + matchSource: "direct", + matchKey: "C_DENIED", + }, + "Slack channel denied by configuration", + ); + }); + + it("repeats disabled-channel warnings on a fixed interval despite steady traffic", () => { + vi.useFakeTimers(); + try { + const ctx = createSlackMonitorContext({ + ...baseParams(), + groupPolicy: "open", + channelsConfig: { C_DENIED: { enabled: false } }, + }); + const warnSpy = vi.spyOn(ctx.logger, "warn").mockImplementation(() => undefined); + + expect(ctx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe(false); + vi.advanceTimersByTime(4 * 60_000); + expect(ctx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe(false); + expect(warnSpy).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(60_000); + expect(ctx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe(false); + expect(warnSpy).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + + it("warns for wildcard disablement under allowlist policy", () => { + const ctx = createSlackMonitorContext({ + ...baseParams(), + groupPolicy: "allowlist", + channelsConfig: { "*": { enabled: false } }, + }); + const warnSpy = vi.spyOn(ctx.logger, "warn").mockImplementation(() => undefined); + + expect(ctx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe(false); + + expect(warnSpy).toHaveBeenCalledExactlyOnceWith( + { + provider: "slack", + accountId: "default", + channelId: "C_DENIED", + reason: "channel_not_allowed", + cause: "channel_disabled", + groupPolicy: "allowlist", + matchSource: "wildcard", + matchKey: "*", + }, + "Slack channel denied by configuration", + ); + }); + + it("does not warn for allowlist misses or globally disabled groups", () => { + const allowlistCtx = createListedChannelsContext("allowlist"); + const allowlistWarnSpy = vi + .spyOn(allowlistCtx.logger, "warn") + .mockImplementation(() => undefined); + expect(allowlistCtx.isChannelAllowed({ channelId: "C_UNLISTED", channelType: "channel" })).toBe( + false, + ); + expect(allowlistWarnSpy).not.toHaveBeenCalled(); + + const disabledCtx = createSlackMonitorContext({ + ...baseParams(), + groupPolicy: "disabled", + channelsConfig: { C_DENIED: { enabled: false } }, + }); + const disabledWarnSpy = vi + .spyOn(disabledCtx.logger, "warn") + .mockImplementation(() => undefined); + expect(disabledCtx.isChannelAllowed({ channelId: "C_DENIED", channelType: "channel" })).toBe( + false, + ); + expect(disabledWarnSpy).not.toHaveBeenCalled(); + }); + it("allows all channels when groupPolicy is open and channelsConfig is empty", () => { const ctx = createSlackMonitorContext({ ...baseParams(),