From 5a32a66aa8acedf7da61a0c58b2b28905436d337 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 21:07:34 +0000 Subject: [PATCH] perf(core): speed up routing, pairing, slack, and security scans --- src/pairing/pairing-store.test.ts | 66 ++++++++- src/pairing/pairing-store.ts | 135 ++++++++++++++++++ src/plugins/manifest-registry.ts | 25 +++- src/routing/resolve-route.ts | 142 ++++++++++++++++++- src/security/audit-extra.async.ts | 72 ++++++++-- src/security/audit.ts | 16 ++- src/slack/monitor/auth.ts | 1 + src/slack/monitor/channel-config.ts | 5 +- src/slack/monitor/context.ts | 4 + src/slack/monitor/message-handler/prepare.ts | 30 +++- src/slack/monitor/slash.ts | 4 +- 11 files changed, 462 insertions(+), 38 deletions(-) diff --git a/src/pairing/pairing-store.test.ts b/src/pairing/pairing-store.test.ts index 34752372090..e0127d664b2 100644 --- a/src/pairing/pairing-store.test.ts +++ b/src/pairing/pairing-store.test.ts @@ -1,13 +1,15 @@ import crypto from "node:crypto"; +import fsSync from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { resolveOAuthDir } from "../config/paths.js"; import { DEFAULT_ACCOUNT_ID } from "../routing/session-key.js"; import { withEnvAsync } from "../test-utils/env.js"; import { addChannelAllowFromStoreEntry, + clearPairingAllowFromReadCacheForTest, approveChannelPairingCode, listChannelPairingRequests, readChannelAllowFromStore, @@ -31,6 +33,10 @@ afterAll(async () => { } }); +beforeEach(() => { + clearPairingAllowFromReadCacheForTest(); +}); + async function withTempStateDir(fn: (stateDir: string) => Promise) { const dir = path.join(fixtureRoot, `case-${caseId++}`); await fs.mkdir(dir, { recursive: true }); @@ -412,4 +418,62 @@ describe("pairing store", () => { expect(syncScoped).toEqual(["1002", "1001"]); }); }); + + it("reuses cached async allowFrom reads and invalidates on file updates", async () => { + await withTempStateDir(async (stateDir) => { + await writeAllowFromFixture({ + stateDir, + channel: "telegram", + accountId: "yy", + allowFrom: ["1001"], + }); + const readSpy = vi.spyOn(fs, "readFile"); + + const first = await readChannelAllowFromStore("telegram", process.env, "yy"); + const second = await readChannelAllowFromStore("telegram", process.env, "yy"); + expect(first).toEqual(["1001"]); + expect(second).toEqual(["1001"]); + expect(readSpy).toHaveBeenCalledTimes(1); + + await writeAllowFromFixture({ + stateDir, + channel: "telegram", + accountId: "yy", + allowFrom: ["1002"], + }); + const third = await readChannelAllowFromStore("telegram", process.env, "yy"); + expect(third).toEqual(["1002"]); + expect(readSpy).toHaveBeenCalledTimes(2); + readSpy.mockRestore(); + }); + }); + + it("reuses cached sync allowFrom reads and invalidates on file updates", async () => { + await withTempStateDir(async (stateDir) => { + await writeAllowFromFixture({ + stateDir, + channel: "telegram", + accountId: "yy", + allowFrom: ["1001"], + }); + const readSpy = vi.spyOn(fsSync, "readFileSync"); + + const first = readChannelAllowFromStoreSync("telegram", process.env, "yy"); + const second = readChannelAllowFromStoreSync("telegram", process.env, "yy"); + expect(first).toEqual(["1001"]); + expect(second).toEqual(["1001"]); + expect(readSpy).toHaveBeenCalledTimes(1); + + await writeAllowFromFixture({ + stateDir, + channel: "telegram", + accountId: "yy", + allowFrom: ["1002"], + }); + const third = readChannelAllowFromStoreSync("telegram", process.env, "yy"); + expect(third).toEqual(["1002"]); + expect(readSpy).toHaveBeenCalledTimes(2); + readSpy.mockRestore(); + }); + }); }); diff --git a/src/pairing/pairing-store.ts b/src/pairing/pairing-store.ts index 467a52d0572..a39b90b8f47 100644 --- a/src/pairing/pairing-store.ts +++ b/src/pairing/pairing-store.ts @@ -24,6 +24,14 @@ const PAIRING_STORE_LOCK_OPTIONS = { }, stale: 30_000, } as const; +type AllowFromReadCacheEntry = { + exists: boolean; + mtimeMs: number | null; + size: number | null; + entries: string[]; +}; + +const allowFromReadCache = new Map(); export type PairingChannel = ChannelId; @@ -278,15 +286,86 @@ async function readAllowFromStateForPath( return (await readAllowFromStateForPathWithExists(channel, filePath)).entries; } +function cloneAllowFromCacheEntry(entry: AllowFromReadCacheEntry): AllowFromReadCacheEntry { + return { + exists: entry.exists, + mtimeMs: entry.mtimeMs, + size: entry.size, + entries: entry.entries.slice(), + }; +} + +function setAllowFromReadCache(filePath: string, entry: AllowFromReadCacheEntry): void { + allowFromReadCache.set(filePath, cloneAllowFromCacheEntry(entry)); +} + +function resolveAllowFromReadCacheHit(params: { + filePath: string; + exists: boolean; + mtimeMs: number | null; + size: number | null; +}): AllowFromReadCacheEntry | null { + const cached = allowFromReadCache.get(params.filePath); + if (!cached) { + return null; + } + if (cached.exists !== params.exists) { + return null; + } + if (!params.exists) { + return cloneAllowFromCacheEntry(cached); + } + if (cached.mtimeMs !== params.mtimeMs || cached.size !== params.size) { + return null; + } + return cloneAllowFromCacheEntry(cached); +} + async function readAllowFromStateForPathWithExists( channel: PairingChannel, filePath: string, ): Promise<{ entries: string[]; exists: boolean }> { + let stat: Awaited> | null = null; + try { + stat = await fs.promises.stat(filePath); + } catch (err) { + const code = (err as { code?: string }).code; + if (code !== "ENOENT") { + throw err; + } + } + + const cached = resolveAllowFromReadCacheHit({ + filePath, + exists: Boolean(stat), + mtimeMs: stat?.mtimeMs ?? null, + size: stat?.size ?? null, + }); + if (cached) { + return { entries: cached.entries, exists: cached.exists }; + } + + if (!stat) { + setAllowFromReadCache(filePath, { + exists: false, + mtimeMs: null, + size: null, + entries: [], + }); + return { entries: [], exists: false }; + } + const { value, exists } = await readJsonFile(filePath, { version: 1, allowFrom: [], }); const entries = normalizeAllowFromList(channel, value); + setAllowFromReadCache(filePath, { + exists, + mtimeMs: stat.mtimeMs, + size: stat.size, + entries, + }); return { entries, exists }; } @@ -298,6 +377,36 @@ function readAllowFromStateForPathSyncWithExists( channel: PairingChannel, filePath: string, ): { entries: string[]; exists: boolean } { + let stat: fs.Stats | null = null; + try { + stat = fs.statSync(filePath); + } catch (err) { + const code = (err as { code?: string }).code; + if (code !== "ENOENT") { + return { entries: [], exists: false }; + } + } + + const cached = resolveAllowFromReadCacheHit({ + filePath, + exists: Boolean(stat), + mtimeMs: stat?.mtimeMs ?? null, + size: stat?.size ?? null, + }); + if (cached) { + return { entries: cached.entries, exists: cached.exists }; + } + + if (!stat) { + setAllowFromReadCache(filePath, { + exists: false, + mtimeMs: null, + size: null, + entries: [], + }); + return { entries: [], exists: false }; + } + let raw = ""; try { raw = fs.readFileSync(filePath, "utf8"); @@ -311,9 +420,21 @@ function readAllowFromStateForPathSyncWithExists( try { const parsed = JSON.parse(raw) as AllowFromStore; const entries = normalizeAllowFromList(channel, parsed); + setAllowFromReadCache(filePath, { + exists: true, + mtimeMs: stat.mtimeMs, + size: stat.size, + entries, + }); return { entries, exists: true }; } catch { // Keep parity with async reads: malformed JSON still means the file exists. + setAllowFromReadCache(filePath, { + exists: true, + mtimeMs: stat.mtimeMs, + size: stat.size, + entries: [], + }); return { entries: [], exists: true }; } } @@ -337,6 +458,16 @@ async function writeAllowFromState(filePath: string, allowFrom: string[]): Promi version: 1, allowFrom, } satisfies AllowFromStore); + let stat: Awaited> | null = null; + try { + stat = await fs.promises.stat(filePath); + } catch {} + setAllowFromReadCache(filePath, { + exists: true, + mtimeMs: stat?.mtimeMs ?? null, + size: stat?.size ?? null, + entries: allowFrom.slice(), + }); } async function readNonDefaultAccountAllowFrom(params: { @@ -448,6 +579,10 @@ export function readChannelAllowFromStoreSync( return dedupePreserveOrder([...scopedEntries, ...legacyEntries]); } +export function clearPairingAllowFromReadCacheForTest(): void { + allowFromReadCache.clear(); +} + type AllowFromStoreEntryUpdateParams = { channel: PairingChannel; entry: string | number; diff --git a/src/plugins/manifest-registry.ts b/src/plugins/manifest-registry.ts index 80313e99fd6..ea93e2d5725 100644 --- a/src/plugins/manifest-registry.ts +++ b/src/plugins/manifest-registry.ts @@ -188,19 +188,30 @@ export function loadPluginManifestRegistry(params: { } const configSchema = manifest.configSchema; - const manifestMtime = safeStatMtimeMs(manifestRes.manifestPath); - const schemaCacheKey = manifestMtime - ? `${manifestRes.manifestPath}:${manifestMtime}` - : manifestRes.manifestPath; + const schemaCacheKey = (() => { + if (!configSchema) { + return undefined; + } + const manifestMtime = safeStatMtimeMs(manifestRes.manifestPath); + return manifestMtime + ? `${manifestRes.manifestPath}:${manifestMtime}` + : manifestRes.manifestPath; + })(); const existing = seenIds.get(manifest.id); if (existing) { // Check whether both candidates point to the same physical directory // (e.g. via symlinks or different path representations). If so, this // is a false-positive duplicate and can be silently skipped. - const existingReal = safeRealpathSync(existing.candidate.rootDir, realpathCache); - const candidateReal = safeRealpathSync(candidate.rootDir, realpathCache); - const samePlugin = Boolean(existingReal && candidateReal && existingReal === candidateReal); + const samePath = existing.candidate.rootDir === candidate.rootDir; + const samePlugin = (() => { + if (samePath) { + return true; + } + const existingReal = safeRealpathSync(existing.candidate.rootDir, realpathCache); + const candidateReal = safeRealpathSync(candidate.rootDir, realpathCache); + return Boolean(existingReal && candidateReal && existingReal === candidateReal); + })(); if (samePlugin) { // Prefer higher-precedence origins even if candidates are passed in // an unexpected order (config > workspace > global > bundled). diff --git a/src/routing/resolve-route.ts b/src/routing/resolve-route.ts index 1d6c8a93772..307315e6e18 100644 --- a/src/routing/resolve-route.ts +++ b/src/routing/resolve-route.ts @@ -199,11 +199,116 @@ type BindingScope = { type EvaluatedBindingsCache = { bindingsRef: OpenClawConfig["bindings"]; byChannelAccount: Map; + byChannelAccountIndex: Map; }; const evaluatedBindingsCacheByCfg = new WeakMap(); const MAX_EVALUATED_BINDINGS_CACHE_KEYS = 2000; +type EvaluatedBindingsIndex = { + byPeer: Map; + byGuildWithRoles: Map; + byGuild: Map; + byTeam: Map; + byAccount: EvaluatedBinding[]; + byChannel: EvaluatedBinding[]; +}; + +function pushToIndexMap( + map: Map, + key: string | null, + binding: EvaluatedBinding, +): void { + if (!key) { + return; + } + const existing = map.get(key); + if (existing) { + existing.push(binding); + return; + } + map.set(key, [binding]); +} + +function peerLookupKeys(kind: ChatType, id: string): string[] { + if (kind === "group") { + return [`group:${id}`, `channel:${id}`]; + } + if (kind === "channel") { + return [`channel:${id}`, `group:${id}`]; + } + return [`${kind}:${id}`]; +} + +function collectPeerIndexedBindings( + index: EvaluatedBindingsIndex, + peer: RoutePeer | null, +): EvaluatedBinding[] { + if (!peer) { + return []; + } + const out: EvaluatedBinding[] = []; + const seen = new Set(); + for (const key of peerLookupKeys(peer.kind, peer.id)) { + const matches = index.byPeer.get(key); + if (!matches) { + continue; + } + for (const match of matches) { + if (seen.has(match)) { + continue; + } + seen.add(match); + out.push(match); + } + } + return out; +} + +function buildEvaluatedBindingsIndex(bindings: EvaluatedBinding[]): EvaluatedBindingsIndex { + const byPeer = new Map(); + const byGuildWithRoles = new Map(); + const byGuild = new Map(); + const byTeam = new Map(); + const byAccount: EvaluatedBinding[] = []; + const byChannel: EvaluatedBinding[] = []; + + for (const binding of bindings) { + if (binding.match.peer.state === "valid") { + for (const key of peerLookupKeys(binding.match.peer.kind, binding.match.peer.id)) { + pushToIndexMap(byPeer, key, binding); + } + continue; + } + if (binding.match.guildId && binding.match.roles) { + pushToIndexMap(byGuildWithRoles, binding.match.guildId, binding); + continue; + } + if (binding.match.guildId && !binding.match.roles) { + pushToIndexMap(byGuild, binding.match.guildId, binding); + continue; + } + if (binding.match.teamId) { + pushToIndexMap(byTeam, binding.match.teamId, binding); + continue; + } + if (binding.match.accountPattern !== "*") { + byAccount.push(binding); + continue; + } + byChannel.push(binding); + } + + return { + byPeer, + byGuildWithRoles, + byGuild, + byTeam, + byAccount, + byChannel, + }; +} + function getEvaluatedBindingsForChannelAccount( cfg: OpenClawConfig, channel: string, @@ -214,7 +319,11 @@ function getEvaluatedBindingsForChannelAccount( const cache = existing && existing.bindingsRef === bindingsRef ? existing - : { bindingsRef, byChannelAccount: new Map() }; + : { + bindingsRef, + byChannelAccount: new Map(), + byChannelAccountIndex: new Map(), + }; if (cache !== existing) { evaluatedBindingsCacheByCfg.set(cfg, cache); } @@ -239,14 +348,34 @@ function getEvaluatedBindingsForChannelAccount( }); cache.byChannelAccount.set(cacheKey, evaluated); + cache.byChannelAccountIndex.set(cacheKey, buildEvaluatedBindingsIndex(evaluated)); if (cache.byChannelAccount.size > MAX_EVALUATED_BINDINGS_CACHE_KEYS) { cache.byChannelAccount.clear(); + cache.byChannelAccountIndex.clear(); cache.byChannelAccount.set(cacheKey, evaluated); + cache.byChannelAccountIndex.set(cacheKey, buildEvaluatedBindingsIndex(evaluated)); } return evaluated; } +function getEvaluatedBindingIndexForChannelAccount( + cfg: OpenClawConfig, + channel: string, + accountId: string, +): EvaluatedBindingsIndex { + const bindings = getEvaluatedBindingsForChannelAccount(cfg, channel, accountId); + const existing = evaluatedBindingsCacheByCfg.get(cfg); + const cacheKey = `${channel}\t${accountId}`; + const indexed = existing?.byChannelAccountIndex.get(cacheKey); + if (indexed) { + return indexed; + } + const built = buildEvaluatedBindingsIndex(bindings); + existing?.byChannelAccountIndex.set(cacheKey, built); + return built; +} + function normalizePeerConstraint( peer: { kind?: string; id?: string } | undefined, ): NormalizedPeerConstraint { @@ -347,6 +476,7 @@ export function resolveAgentRoute(input: ResolveAgentRouteInput): ResolvedAgentR const memberRoleIdSet = new Set(memberRoleIds); const bindings = getEvaluatedBindingsForChannelAccount(input.cfg, channel, accountId); + const bindingsIndex = getEvaluatedBindingIndexForChannelAccount(input.cfg, channel, accountId); const dmScope = input.cfg.session?.dmScope ?? "main"; const identityLinks = input.cfg.session?.identityLinks; @@ -415,24 +545,28 @@ export function resolveAgentRoute(input: ResolveAgentRouteInput): ResolvedAgentR matchedBy: Exclude; enabled: boolean; scopePeer: RoutePeer | null; + candidates: EvaluatedBinding[]; predicate: (candidate: EvaluatedBinding) => boolean; }> = [ { matchedBy: "binding.peer", enabled: Boolean(peer), scopePeer: peer, + candidates: collectPeerIndexedBindings(bindingsIndex, peer), predicate: (candidate) => candidate.match.peer.state === "valid", }, { matchedBy: "binding.peer.parent", enabled: Boolean(parentPeer && parentPeer.id), scopePeer: parentPeer && parentPeer.id ? parentPeer : null, + candidates: collectPeerIndexedBindings(bindingsIndex, parentPeer), predicate: (candidate) => candidate.match.peer.state === "valid", }, { matchedBy: "binding.guild+roles", enabled: Boolean(guildId && memberRoleIds.length > 0), scopePeer: peer, + candidates: guildId ? (bindingsIndex.byGuildWithRoles.get(guildId) ?? []) : [], predicate: (candidate) => hasGuildConstraint(candidate.match) && hasRolesConstraint(candidate.match), }, @@ -440,6 +574,7 @@ export function resolveAgentRoute(input: ResolveAgentRouteInput): ResolvedAgentR matchedBy: "binding.guild", enabled: Boolean(guildId), scopePeer: peer, + candidates: guildId ? (bindingsIndex.byGuild.get(guildId) ?? []) : [], predicate: (candidate) => hasGuildConstraint(candidate.match) && !hasRolesConstraint(candidate.match), }, @@ -447,18 +582,21 @@ export function resolveAgentRoute(input: ResolveAgentRouteInput): ResolvedAgentR matchedBy: "binding.team", enabled: Boolean(teamId), scopePeer: peer, + candidates: teamId ? (bindingsIndex.byTeam.get(teamId) ?? []) : [], predicate: (candidate) => hasTeamConstraint(candidate.match), }, { matchedBy: "binding.account", enabled: true, scopePeer: peer, + candidates: bindingsIndex.byAccount, predicate: (candidate) => candidate.match.accountPattern !== "*", }, { matchedBy: "binding.channel", enabled: true, scopePeer: peer, + candidates: bindingsIndex.byChannel, predicate: (candidate) => candidate.match.accountPattern === "*", }, ]; @@ -467,7 +605,7 @@ export function resolveAgentRoute(input: ResolveAgentRouteInput): ResolvedAgentR if (!tier.enabled) { continue; } - const matched = bindings.find( + const matched = tier.candidates.find( (candidate) => tier.predicate(candidate) && matchesBindingScope(candidate.match, { diff --git a/src/security/audit-extra.async.ts b/src/security/audit-extra.async.ts index 8fecfdd039d..11b0f98f7f1 100644 --- a/src/security/audit-extra.async.ts +++ b/src/security/audit-extra.async.ts @@ -52,6 +52,8 @@ type ExecDockerRawFn = ( opts?: { allowFailure?: boolean; input?: Buffer | string; signal?: AbortSignal }, ) => Promise; +type CodeSafetySummaryCache = Map>; + // -------------------------------------------------------------------------- // Helpers // -------------------------------------------------------------------------- @@ -246,6 +248,41 @@ async function readInstalledPackageVersion(dir: string): Promise entry.trim()).filter(Boolean); + const includeKey = includeFiles.length > 0 ? includeFiles.toSorted().join("\u0000") : ""; + return `${params.dirPath}\u0000${includeKey}`; +} + +async function getCodeSafetySummary(params: { + dirPath: string; + includeFiles?: string[]; + summaryCache?: CodeSafetySummaryCache; +}): Promise>> { + const cacheKey = buildCodeSafetySummaryCacheKey({ + dirPath: params.dirPath, + includeFiles: params.includeFiles, + }); + const cache = params.summaryCache; + if (cache) { + const hit = cache.get(cacheKey); + if (hit) { + return (await hit) as Awaited>; + } + const pending = skillScanner.scanDirectoryWithSummary(params.dirPath, { + includeFiles: params.includeFiles, + }); + cache.set(cacheKey, pending); + return await pending; + } + return await skillScanner.scanDirectoryWithSummary(params.dirPath, { + includeFiles: params.includeFiles, + }); +} + // -------------------------------------------------------------------------- // Exported collectors // -------------------------------------------------------------------------- @@ -965,6 +1002,7 @@ export async function readConfigSnapshotForAudit(params: { export async function collectPluginsCodeSafetyFindings(params: { stateDir: string; + summaryCache?: CodeSafetySummaryCache; }): Promise { const findings: SecurityAuditFinding[] = []; const { extensionsDir, pluginDirs } = await listInstalledPluginDirs({ @@ -1016,21 +1054,21 @@ export async function collectPluginsCodeSafetyFindings(params: { }); } - const summary = await skillScanner - .scanDirectoryWithSummary(pluginPath, { - includeFiles: forcedScanEntries, - }) - .catch((err) => { - findings.push({ - checkId: "plugins.code_safety.scan_failed", - severity: "warn", - title: `Plugin "${pluginName}" code scan failed`, - detail: `Static code scan could not complete: ${String(err)}`, - remediation: - "Check file permissions and plugin layout, then rerun `openclaw security audit --deep`.", - }); - return null; + const summary = await getCodeSafetySummary({ + dirPath: pluginPath, + includeFiles: forcedScanEntries, + summaryCache: params.summaryCache, + }).catch((err) => { + findings.push({ + checkId: "plugins.code_safety.scan_failed", + severity: "warn", + title: `Plugin "${pluginName}" code scan failed`, + detail: `Static code scan could not complete: ${String(err)}`, + remediation: + "Check file permissions and plugin layout, then rerun `openclaw security audit --deep`.", }); + return null; + }); if (!summary) { continue; } @@ -1067,6 +1105,7 @@ export async function collectPluginsCodeSafetyFindings(params: { export async function collectInstalledSkillsCodeSafetyFindings(params: { cfg: OpenClawConfig; stateDir: string; + summaryCache?: CodeSafetySummaryCache; }): Promise { const findings: SecurityAuditFinding[] = []; const pluginExtensionsDir = path.join(params.stateDir, "extensions"); @@ -1091,7 +1130,10 @@ export async function collectInstalledSkillsCodeSafetyFindings(params: { scannedSkillDirs.add(skillDir); const skillName = entry.skill.name; - const summary = await skillScanner.scanDirectoryWithSummary(skillDir).catch((err) => { + const summary = await getCodeSafetySummary({ + dirPath: skillDir, + summaryCache: params.summaryCache, + }).catch((err) => { findings.push({ checkId: "skills.code_safety.scan_failed", severity: "warn", diff --git a/src/security/audit.ts b/src/security/audit.ts index 749b0fe6b22..fbc1fcb322e 100644 --- a/src/security/audit.ts +++ b/src/security/audit.ts @@ -1036,6 +1036,7 @@ export async function runSecurityAudit(opts: SecurityAuditOptions): Promise>(); findings.push( ...(await collectFilesystemFindings({ stateDir, @@ -1060,8 +1061,19 @@ export async function runSecurityAudit(opts: SecurityAuditOptions): Promise 0; + const channelsConfigKeys = Object.keys(params.channelsConfig ?? {}); const markMessageSeen = (channelId: string | undefined, ts?: string) => { if (!channelId || !ts) { @@ -331,6 +333,7 @@ export function createSlackMonitorContext(params: { channelId: p.channelId, channelName: p.channelName, channels: params.channelsConfig, + channelKeys: channelsConfigKeys, defaultRequireMention, }); const channelMatchMeta = formatAllowlistMatchMeta(channelConfig); @@ -413,6 +416,7 @@ export function createSlackMonitorContext(params: { groupDmChannels, defaultRequireMention, channelsConfig: params.channelsConfig, + channelsConfigKeys, groupPolicy: params.groupPolicy, useAccessGroups: params.useAccessGroups, reactionMode: params.reactionMode, diff --git a/src/slack/monitor/message-handler/prepare.ts b/src/slack/monitor/message-handler/prepare.ts index b83b19c18d1..0369b267689 100644 --- a/src/slack/monitor/message-handler/prepare.ts +++ b/src/slack/monitor/message-handler/prepare.ts @@ -108,6 +108,7 @@ export async function prepareSlackMessage(params: { channelId: message.channel, channelName, channels: ctx.channelsConfig, + channelKeys: ctx.channelsConfigKeys, defaultRequireMention: ctx.defaultRequireMention, }) : null; @@ -251,15 +252,29 @@ export async function prepareSlackMessage(params: { hasSlackThreadParticipation(account.accountId, message.channel, message.thread_ts)), ); - const sender = message.user ? await ctx.resolveUserName(message.user) : null; - const senderName = - sender?.name ?? message.username?.trim() ?? message.user ?? message.bot_id ?? "unknown"; + let resolvedSenderName = message.username?.trim() || undefined; + const resolveSenderName = async (): Promise => { + if (resolvedSenderName) { + return resolvedSenderName; + } + if (message.user) { + const sender = await ctx.resolveUserName(message.user); + const normalized = sender?.name?.trim(); + if (normalized) { + resolvedSenderName = normalized; + return resolvedSenderName; + } + } + resolvedSenderName = message.user ?? message.bot_id ?? "unknown"; + return resolvedSenderName; + }; + const senderNameForAuth = ctx.allowNameMatching ? await resolveSenderName() : undefined; const channelUserAuthorized = isRoom ? resolveSlackUserAllowed({ allowList: channelConfig?.users, userId: senderId, - userName: senderName, + userName: senderNameForAuth, allowNameMatching: ctx.allowNameMatching, }) : true; @@ -279,7 +294,7 @@ export async function prepareSlackMessage(params: { const ownerAuthorized = resolveSlackAllowListMatch({ allowList: allowFromLower, id: senderId, - name: senderName, + name: senderNameForAuth, allowNameMatching: ctx.allowNameMatching, }).allowed; const channelUsersAllowlistConfigured = @@ -289,7 +304,7 @@ export async function prepareSlackMessage(params: { ? resolveSlackUserAllowed({ allowList: channelConfig?.users, userId: senderId, - userName: senderName, + userName: senderNameForAuth, allowNameMatching: ctx.allowNameMatching, }) : false; @@ -350,7 +365,7 @@ export async function prepareSlackMessage(params: { limit: ctx.historyLimit, entry: pendingBody ? { - sender: senderName, + sender: await resolveSenderName(), body: pendingBody, timestamp: message.ts ? Math.round(Number(message.ts) * 1000) : undefined, messageId: message.ts, @@ -455,6 +470,7 @@ export async function prepareSlackMessage(params: { : null; const roomLabel = channelName ? `#${channelName}` : `#${message.channel}`; + const senderName = await resolveSenderName(); const preview = rawBody.replace(/\s+/g, " ").slice(0, 160); const inboundLabel = isDirectMessage ? `Slack DM from ${senderName}` diff --git a/src/slack/monitor/slash.ts b/src/slack/monitor/slash.ts index dcd379da680..596ca83ba93 100644 --- a/src/slack/monitor/slash.ts +++ b/src/slack/monitor/slash.ts @@ -385,11 +385,11 @@ export async function registerSlackMonitorSlashCommands(params: { channelId: command.channel_id, channelName: channelInfo?.name, channels: ctx.channelsConfig, + channelKeys: ctx.channelsConfigKeys, defaultRequireMention: ctx.defaultRequireMention, }); if (ctx.useAccessGroups) { - const channelAllowlistConfigured = - Boolean(ctx.channelsConfig) && Object.keys(ctx.channelsConfig ?? {}).length > 0; + const channelAllowlistConfigured = (ctx.channelsConfigKeys?.length ?? 0) > 0; const channelAllowed = channelConfig?.allowed !== false; if ( !isSlackChannelAllowedByPolicy({