diff --git a/.changeset/fix-mention-offset-user-change.md b/.changeset/fix-mention-offset-user-change.md new file mode 100644 index 00000000..04315ca9 --- /dev/null +++ b/.changeset/fix-mention-offset-user-change.md @@ -0,0 +1,5 @@ +--- +"@chat-adapter/slack": patch +--- + +Fix duplicate mention resolution by using the replace callback offset instead of indexOf. Invalidate user cache on Slack user_change events so display name updates are picked up immediately. diff --git a/packages/adapter-slack/src/index.test.ts b/packages/adapter-slack/src/index.test.ts index ffe7a4b2..91cd0989 100644 --- a/packages/adapter-slack/src/index.test.ts +++ b/packages/adapter-slack/src/index.test.ts @@ -1173,6 +1173,26 @@ function createMockState(): StateAdapter & { cache: Map } { cache.delete(key); return Promise.resolve(); }), + appendToList: vi + .fn() + .mockImplementation( + ( + key: string, + value: unknown, + options?: { maxLength?: number; ttlMs?: number } + ) => { + let list = (cache.get(key) as unknown[]) ?? []; + list.push(value); + if (options?.maxLength && list.length > options.maxLength) { + list = list.slice(list.length - options.maxLength); + } + cache.set(key, list); + return Promise.resolve(); + } + ), + getList: vi.fn().mockImplementation((key: string) => { + return Promise.resolve((cache.get(key) as unknown[]) ?? []); + }), }; } @@ -4598,3 +4618,367 @@ describe("isMessageFromSelf", () => { expect(result).toBe(false); }); }); + +// ============================================================================ +// Reverse User Lookup — @mention Resolution Tests +// ============================================================================ + +describe("reverse user lookup", () => { + const secret = "test-signing-secret"; + + interface MentionAdapter { + chat: ChatInstance | null; + resolveMessageMentions( + message: AdapterPostableMessage, + threadId: string + ): Promise; + resolveOutgoingMentions(text: string, threadId: string): Promise; + } + + function createAdapterWithState() { + const state = createMockState(); + const adapter = createSlackAdapter({ + botToken: "xoxb-test-token", + signingSecret: secret, + logger: mockLogger, + }); + const chatInstance = createMockChatInstance(state); + (adapter as unknown as MentionAdapter).chat = chatInstance; + return { adapter, state }; + } + + describe("reverse index storage in lookupUser", () => { + it("stores reverse index when looking up a user", async () => { + const { adapter, state } = createAdapterWithState(); + + // Mock Slack API + const mockClient = ( + adapter as unknown as { client: { users: { info: unknown } } } + ).client; + mockClient.users.info = vi.fn().mockResolvedValue({ + user: { + profile: { display_name: "dominik", real_name: "Dominik G" }, + real_name: "Dominik G", + name: "dominik", + }, + }); + + await ( + adapter as unknown as { + lookupUser( + userId: string + ): Promise<{ displayName: string; realName: string }>; + } + ).lookupUser("U_DOM_123"); + + // Check reverse index was written + const userIds = await state.getList("slack:user-by-name:dominik"); + expect(userIds).toContain("U_DOM_123"); + }); + }); + + describe("resolveOutgoingMentions", () => { + it("resolves unambiguous @mention to <@USER_ID>", async () => { + const { adapter, state } = createAdapterWithState(); + + // Seed reverse index: "dominik" → ["U_DOM_123"] + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions( + "Hey @dominik, check this out", + "slack:C123:1234567890.123456" + ); + + expect(result).toBe("Hey <@U_DOM_123>, check this out"); + }); + + it("handles case insensitivity", async () => { + const { adapter, state } = createAdapterWithState(); + + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions( + "Hey @Dominik!", + "slack:C123:1234567890.123456" + ); + + expect(result).toBe("Hey <@U_DOM_123>!"); + }); + + it("deduplicates user IDs from reverse index", async () => { + const { adapter, state } = createAdapterWithState(); + + // Same user ID appended multiple times (from repeated lookups) + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions("Hey @dominik", "slack:C123:1234567890.123456"); + + expect(result).toBe("Hey <@U_DOM_123>"); + }); + + it("leaves mention as plain text when no match found", async () => { + const { adapter } = createAdapterWithState(); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions( + "Hey @unknown_user", + "slack:C123:1234567890.123456" + ); + + expect(result).toBe("Hey @unknown_user"); + }); + + it("skips already-resolved <@USER_ID> mentions", async () => { + const { adapter, state } = createAdapterWithState(); + + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions( + "Hey <@U_DOM_123> and @dominik", + "slack:C123:1234567890.123456" + ); + + expect(result).toContain("<@U_DOM_123>"); + // The second @dominik should also be resolved + expect(result).toBe("Hey <@U_DOM_123> and <@U_DOM_123>"); + }); + + it("disambiguates using thread participants", async () => { + const { adapter, state } = createAdapterWithState(); + const threadId = "slack:C123:1234567890.123456"; + + // Two users named "alex" + await state.appendToList("slack:user-by-name:alex", "U_ALEX_1"); + await state.appendToList("slack:user-by-name:alex", "U_ALEX_2"); + + // Only U_ALEX_2 is a thread participant + await state.appendToList( + `slack:thread-participants:${threadId}`, + "U_ALEX_2" + ); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions("Hey @alex", threadId); + + expect(result).toBe("Hey <@U_ALEX_2>"); + }); + + it("leaves ambiguous mentions as plain text when thread participants don't help", async () => { + const { adapter, state } = createAdapterWithState(); + const threadId = "slack:C123:1234567890.123456"; + + // Two users named "alex" + await state.appendToList("slack:user-by-name:alex", "U_ALEX_1"); + await state.appendToList("slack:user-by-name:alex", "U_ALEX_2"); + + // Both are thread participants + await state.appendToList( + `slack:thread-participants:${threadId}`, + "U_ALEX_1" + ); + await state.appendToList( + `slack:thread-participants:${threadId}`, + "U_ALEX_2" + ); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions("Hey @alex", threadId); + + expect(result).toBe("Hey @alex"); + }); + + it("resolves multiple different mentions in one message", async () => { + const { adapter, state } = createAdapterWithState(); + + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + await state.appendToList("slack:user-by-name:malte", "U_MAL_456"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions( + "@dominik and @malte please review", + "slack:C123:1234567890.123456" + ); + + expect(result).toBe("<@U_DOM_123> and <@U_MAL_456> please review"); + }); + + it("does nothing when chat is not initialized", async () => { + const adapter = createSlackAdapter({ + botToken: "xoxb-test-token", + signingSecret: secret, + logger: mockLogger, + }); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveOutgoingMentions("Hey @dominik", "slack:C123:1234567890.123456"); + + expect(result).toBe("Hey @dominik"); + }); + }); + + describe("resolveMessageMentions", () => { + it("resolves mentions in string messages", async () => { + const { adapter, state } = createAdapterWithState(); + + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveMessageMentions( + "Hey @dominik" as AdapterPostableMessage, + "slack:C123:1234567890.123456" + ); + + expect(result).toBe("Hey <@U_DOM_123>"); + }); + + it("resolves mentions in raw messages", async () => { + const { adapter, state } = createAdapterWithState(); + + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveMessageMentions( + { raw: "Hey @dominik" } as AdapterPostableMessage, + "slack:C123:1234567890.123456" + ); + + expect(result).toEqual({ raw: "Hey <@U_DOM_123>" }); + }); + + it("resolves mentions in markdown messages", async () => { + const { adapter, state } = createAdapterWithState(); + + await state.appendToList("slack:user-by-name:dominik", "U_DOM_123"); + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveMessageMentions( + { markdown: "Hey @dominik" } as AdapterPostableMessage, + "slack:C123:1234567890.123456" + ); + + expect(result).toEqual({ markdown: "Hey <@U_DOM_123>" }); + }); + + it("passes through AST messages unchanged", async () => { + const { adapter } = createAdapterWithState(); + + const astMessage = { + ast: { type: "root", children: [] }, + } as AdapterPostableMessage; + + const result = await ( + adapter as unknown as MentionAdapter + ).resolveMessageMentions(astMessage, "slack:C123:1234567890.123456"); + + expect(result).toBe(astMessage); + }); + }); + + describe("thread participant tracking", () => { + it("tracks thread participants on incoming messages", async () => { + const { adapter, state } = createAdapterWithState(); + + // Mock Slack API so lookupUser doesn't hit real API + const mockClient = ( + adapter as unknown as { client: { users: { info: unknown } } } + ).client; + mockClient.users.info = vi.fn().mockResolvedValue({ + user: { + profile: { display_name: "sender", real_name: "Sender One" }, + real_name: "Sender One", + name: "sender", + }, + }); + + const threadId = "slack:C123:1234567890.123456"; + const event = { + type: "message", + user: "U_SENDER_1", + text: "Hello", + ts: "1234567890.123456", + channel: "C123", + thread_ts: "1234567890.123456", + }; + + // Call parseSlackMessage directly + await ( + adapter as unknown as { + parseSlackMessage( + event: Record, + threadId: string + ): Promise; + } + ).parseSlackMessage(event, threadId); + + // Allow participant tracking to complete + await new Promise((resolve) => setTimeout(resolve, 10)); + + const participants = await state.getList( + `slack:thread-participants:${threadId}` + ); + expect(participants).toContain("U_SENDER_1"); + }); + }); + + describe("user_change event", () => { + it("invalidates user cache on profile change", async () => { + const state = createMockState(); + const adapter = createSlackAdapter({ + botToken: "xoxb-test-token", + signingSecret: secret, + logger: mockLogger, + }); + await adapter.initialize(createMockChatInstance(state)); + + // Seed user cache + await state.set( + "slack:user:U_DOM_123", + { displayName: "dominik", realName: "Dominik G" }, + 8 * 24 * 60 * 60 * 1000 + ); + + const body = JSON.stringify({ + type: "event_callback", + event: { + type: "user_change", + event_ts: "1234567890.123456", + user: { + id: "U_DOM_123", + name: "dominik", + real_name: "Dominik New", + profile: { + display_name: "dom_new", + real_name: "Dominik New", + }, + }, + }, + }); + const request = createWebhookRequest(body, secret); + const response = await adapter.handleWebhook(request); + + expect(response.status).toBe(200); + + await new Promise((resolve) => setTimeout(resolve, 50)); + + const cached = await state.get("slack:user:U_DOM_123"); + expect(cached).toBeNull(); + }); + }); +}); diff --git a/packages/adapter-slack/src/index.ts b/packages/adapter-slack/src/index.ts index 84e302da..ebc419d4 100644 --- a/packages/adapter-slack/src/index.ts +++ b/packages/adapter-slack/src/index.ts @@ -67,6 +67,7 @@ import { } from "./modals"; const SLACK_USER_ID_PATTERN = /^[A-Z0-9_]+$/; +const SLACK_USER_ID_EXACT_PATTERN = /^U[A-Z0-9]+$/; /** Find the next `<@` or `<#` mention in text. */ function findNextMention(text: string): number { @@ -243,6 +244,18 @@ interface SlackMemberJoinedChannelEvent { user: string; } +/** Slack user_change event payload */ +interface SlackUserChangeEvent { + event_ts: string; + type: "user_change"; + user: { + id: string; + name?: string; + real_name?: string; + profile?: { display_name?: string; real_name?: string }; + }; +} + /** Slack webhook payload envelope */ interface SlackWebhookPayload { challenge?: string; @@ -252,7 +265,8 @@ interface SlackWebhookPayload { | SlackAssistantThreadStartedEvent | SlackAssistantContextChangedEvent | SlackAppHomeOpenedEvent - | SlackMemberJoinedChannelEvent; + | SlackMemberJoinedChannelEvent + | SlackUserChangeEvent; event_id?: string; event_time?: number; team_id?: string; @@ -357,8 +371,9 @@ export class SlackAdapter implements Adapter { private _botUserId: string | null = null; private _botId: string | null = null; // Bot app ID (B_xxx) - different from user ID private readonly formatConverter = new SlackFormatConverter(); - private static USER_CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour - private static CHANNEL_CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour + private static USER_CACHE_TTL_MS = 8 * 24 * 60 * 60 * 1000; // 8 days + private static CHANNEL_CACHE_TTL_MS = 8 * 24 * 60 * 60 * 1000; // 8 days + private static REVERSE_INDEX_TTL_MS = 8 * 24 * 60 * 60 * 1000; // 8 days // Multi-workspace support private readonly clientId: string | undefined; @@ -725,6 +740,17 @@ export class SlackAdapter implements Adapter { { displayName, realName }, SlackAdapter.USER_CACHE_TTL_MS ); + + // Build reverse index: display name → user IDs (skip if already present) + const normalizedName = displayName.toLowerCase(); + const reverseKey = `slack:user-by-name:${normalizedName}`; + const existing = await this.chat.getState().getList(reverseKey); + if (!existing.includes(userId)) { + await this.chat.getState().appendToList(reverseKey, userId, { + maxLength: 50, + ttlMs: SlackAdapter.REVERSE_INDEX_TTL_MS, + }); + } } this.logger.debug("Fetched user info", { @@ -903,6 +929,8 @@ export class SlackAdapter implements Adapter { event as SlackMemberJoinedChannelEvent, options ); + } else if (event.type === "user_change") { + this.handleUserChange(event as SlackUserChangeEvent); } } } @@ -1560,6 +1588,21 @@ export class SlackAdapter implements Adapter { ); } + private async handleUserChange(event: SlackUserChangeEvent): Promise { + if (!this.chat) { + return; + } + + try { + await this.chat.getState().delete(`slack:user:${event.user.id}`); + } catch (error) { + this.logger.warn("Failed to invalidate user cache", { + userId: event.user.id, + error, + }); + } + } + /** * Publish a Home tab view for a user. * Slack API: views.publish @@ -1833,6 +1876,28 @@ export class SlackAdapter implements Adapter { fullName = userInfo.realName; } + // Track thread participants for outgoing mention resolution (skip dupes) + if (event.user && this.chat) { + try { + const participantKey = `slack:thread-participants:${threadId}`; + const participants = await this.chat + .getState() + .getList(participantKey); + if (!participants.includes(event.user)) { + await this.chat.getState().appendToList(participantKey, event.user, { + maxLength: 100, + ttlMs: SlackAdapter.REVERSE_INDEX_TTL_MS, + }); + } + } catch (error) { + this.logger.warn("Failed to track thread participant", { + threadId, + userId: event.user, + error, + }); + } + } + // Resolve inline @mentions to display names const text = await this.resolveInlineMentions(rawText, skipSelfMention); @@ -1927,6 +1992,138 @@ export class SlackAdapter implements Adapter { }; } + /** + * Resolve @name mentions in text to Slack <@USER_ID> format using the + * reverse user cache. When multiple users share a display name, prefers + * the one who is a participant in the given thread. + */ + private async resolveOutgoingMentions( + text: string, + threadId: string + ): Promise { + if (!this.chat) { + return text; + } + const state = this.chat.getState(); + + // Find all @word patterns that aren't already wrapped in <@...> + const mentionPattern = /@(\w+)/g; + const mentions = new Map(); + + for (const match of text.matchAll(mentionPattern)) { + const name = match[1]; + // Skip if already a Slack user ID format or inside <@...> + if (SLACK_USER_ID_EXACT_PATTERN.test(name)) { + continue; + } + // Check the character before @ to skip <@...> patterns + const idx = match.index; + if (idx > 0 && text[idx - 1] === "<") { + continue; + } + if (!mentions.has(name.toLowerCase())) { + mentions.set(name.toLowerCase(), []); + } + } + + if (mentions.size === 0) { + return text; + } + + // Look up user IDs for each mentioned name + for (const name of mentions.keys()) { + const userIds = await state.getList(`slack:user-by-name:${name}`); + // Dedup + const unique = [...new Set(userIds)]; + mentions.set(name, unique); + } + + // Load thread participants only if needed (ambiguous mentions) + let participants: Set | null = null; + const needsParticipants = [...mentions.values()].some( + (ids) => ids.length > 1 + ); + if (needsParticipants) { + const participantList = await state.getList( + `slack:thread-participants:${threadId}` + ); + participants = new Set(participantList); + } + + // Replace mentions in text + return text.replace( + mentionPattern, + (match, name: string, offset: number) => { + if (offset > 0 && text[offset - 1] === "<") { + return match; + } + if (SLACK_USER_ID_EXACT_PATTERN.test(name)) { + return match; + } + + const userIds = mentions.get(name.toLowerCase()); + if (!userIds || userIds.length === 0) { + return match; + } + if (userIds.length === 1) { + return `<@${userIds[0]}>`; + } + // Disambiguate using thread participants + if (participants) { + const inThread = userIds.filter((id) => participants.has(id)); + if (inThread.length === 1) { + return `<@${inThread[0]}>`; + } + } + // Still ambiguous — leave as plain text + return match; + } + ); + } + + /** + * Pre-process an outgoing message to resolve @name mentions before rendering. + */ + private async resolveMessageMentions( + message: AdapterPostableMessage, + threadId: string + ): Promise { + if (!this.chat) { + return message; + } + if (typeof message === "string") { + return this.resolveOutgoingMentions(message, threadId); + } + if (typeof message === "object" && message !== null) { + if ( + "raw" in message && + typeof (message as { raw: unknown }).raw === "string" + ) { + return { + ...message, + raw: await this.resolveOutgoingMentions( + (message as { raw: string }).raw, + threadId + ), + }; + } + if ( + "markdown" in message && + typeof (message as { markdown: unknown }).markdown === "string" + ) { + return { + ...message, + markdown: await this.resolveOutgoingMentions( + (message as { markdown: string }).markdown, + threadId + ), + }; + } + } + // AST, Card, or other formats — pass through unchanged + return message; + } + /** * Try to render a message using native Slack table blocks. * Returns blocks + fallback text if the message contains tables, null otherwise. @@ -1961,8 +2158,9 @@ export class SlackAdapter implements Adapter { async postMessage( threadId: string, - message: AdapterPostableMessage + _message: AdapterPostableMessage ): Promise> { + const message = await this.resolveMessageMentions(_message, threadId); const { channel, threadTs } = this.decodeThreadId(threadId); try { @@ -2101,8 +2299,9 @@ export class SlackAdapter implements Adapter { async postEphemeral( threadId: string, userId: string, - message: AdapterPostableMessage + _message: AdapterPostableMessage ): Promise { + const message = await this.resolveMessageMentions(_message, threadId); const { channel, threadTs } = this.decodeThreadId(threadId); try { @@ -2217,9 +2416,10 @@ export class SlackAdapter implements Adapter { async scheduleMessage( threadId: string, - message: AdapterPostableMessage, + _message: AdapterPostableMessage, options: { postAt: Date } ): Promise { + const message = await this.resolveMessageMentions(_message, threadId); const { channel, threadTs } = this.decodeThreadId(threadId); const postAtUnix = Math.floor(options.postAt.getTime() / 1000); @@ -2454,8 +2654,9 @@ export class SlackAdapter implements Adapter { async editMessage( threadId: string, messageId: string, - message: AdapterPostableMessage + _message: AdapterPostableMessage ): Promise> { + const message = await this.resolveMessageMentions(_message, threadId); const ephemeral = this.decodeEphemeralMessageId(messageId); if (ephemeral) { const { threadTs } = this.decodeThreadId(threadId);