From 52c62232929a6b4dafbdc79c9c55c1e2e0792337 Mon Sep 17 00:00:00 2001 From: gavrielc Date: Mon, 20 Apr 2026 11:11:56 +0300 Subject: [PATCH] fix(channels): register onNewMessage(/./) to fix pattern mode in group chats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chat SDK dispatch (per handling-events.mdx) is exclusive and prioritized: subscribed → onSubscribedMessage; unsubscribed + mention → onNewMention; unsubscribed + pattern match → onNewMessage. We never registered the third, so engage_mode='pattern' silently dropped every message in unsubscribed group threads — the SDK simply never surfaced them anywhere. Register chat.onNewMessage(/./, …) and route it through shouldEngage with a new 'new-message' source. Unknown-conversation policy drops for this source (would otherwise flood from every unwired channel the bot can see). mention / mention-sticky wirings ignore 'new-message' — they require an explicit @mention to start a conversation. Pattern wirings evaluate normally. Extracted shouldEngage from a closure to an exported function with an EngageSource type so it's unit-testable. Added 17 tests covering every source × engage-mode combination, unknown-conversation behavior, invalid regex fail-open, and multi-wiring union. Accumulate (ignored_message_policy='accumulate') is still not plumbed — the bridge drops non-engaging messages entirely instead of forwarding them as context-only. That requires a trigger: 0 | 1 field on InboundMessage → router → writeSessionMessage (schema already has the column). Separate change. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/channels/chat-sdk-bridge.test.ts | 129 ++++++++++++++++++- src/channels/chat-sdk-bridge.ts | 181 ++++++++++++++++++--------- 2 files changed, 249 insertions(+), 61 deletions(-) diff --git a/src/channels/chat-sdk-bridge.test.ts b/src/channels/chat-sdk-bridge.test.ts index e71ccb2..3989c26 100644 --- a/src/channels/chat-sdk-bridge.test.ts +++ b/src/channels/chat-sdk-bridge.test.ts @@ -2,12 +2,33 @@ import { describe, expect, it } from 'vitest'; import type { Adapter } from 'chat'; -import { createChatSdkBridge } from './chat-sdk-bridge.js'; +import type { ConversationConfig } from './adapter.js'; +import { createChatSdkBridge, shouldEngage, type EngageSource } from './chat-sdk-bridge.js'; function stubAdapter(partial: Partial): Adapter { return { name: 'stub', ...partial } as unknown as Adapter; } +function cfg(partial: Partial & { engageMode: ConversationConfig['engageMode'] }): ConversationConfig { + return { + platformId: partial.platformId ?? 'C1', + agentGroupId: partial.agentGroupId ?? 'ag-1', + engageMode: partial.engageMode, + engagePattern: partial.engagePattern ?? null, + sessionMode: partial.sessionMode ?? 'shared', + }; +} + +function mapFor(...configs: ConversationConfig[]): Map { + const map = new Map(); + for (const c of configs) { + const existing = map.get(c.platformId); + if (existing) existing.push(c); + else map.set(c.platformId, [c]); + } + return map; +} + describe('createChatSdkBridge', () => { it('omits openDM when the underlying Chat SDK adapter has none', () => { const bridge = createChatSdkBridge({ @@ -36,3 +57,109 @@ describe('createChatSdkBridge', () => { expect(platformId).toBe('stub:user-42'); }); }); + +describe('shouldEngage', () => { + describe('unknown conversation', () => { + const empty = new Map(); + const sources: EngageSource[] = ['subscribed', 'mention', 'dm']; + for (const source of sources) { + it(`forwards for source='${source}' (may be a not-yet-wired group)`, () => { + expect(shouldEngage(empty, 'C1', source, '')).toEqual({ engage: true, stickySubscribe: false }); + }); + } + it("DROPS for source='new-message' (would flood from unwired channels)", () => { + expect(shouldEngage(empty, 'C1', 'new-message', 'hello')).toEqual({ + engage: false, + stickySubscribe: false, + }); + }); + }); + + describe("engageMode='mention'", () => { + const conv = mapFor(cfg({ engageMode: 'mention' })); + it('engages on mention + dm', () => { + expect(shouldEngage(conv, 'C1', 'mention', '').engage).toBe(true); + expect(shouldEngage(conv, 'C1', 'dm', '').engage).toBe(true); + }); + it('does NOT engage on subscribed or new-message (prevents keep-firing + flooding)', () => { + expect(shouldEngage(conv, 'C1', 'subscribed', '').engage).toBe(false); + expect(shouldEngage(conv, 'C1', 'new-message', '').engage).toBe(false); + }); + it('never asks to subscribe', () => { + for (const s of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) { + expect(shouldEngage(conv, 'C1', s, '').stickySubscribe).toBe(false); + } + }); + }); + + describe("engageMode='mention-sticky'", () => { + const conv = mapFor(cfg({ engageMode: 'mention-sticky' })); + it('engages on mention + dm with stickySubscribe=true', () => { + expect(shouldEngage(conv, 'C1', 'mention', '')).toEqual({ engage: true, stickySubscribe: true }); + expect(shouldEngage(conv, 'C1', 'dm', '')).toEqual({ engage: true, stickySubscribe: true }); + }); + it('engages on subscribed follow-ups without re-subscribing', () => { + expect(shouldEngage(conv, 'C1', 'subscribed', '')).toEqual({ engage: true, stickySubscribe: false }); + }); + it('does NOT engage on new-message (explicit mention required to start)', () => { + expect(shouldEngage(conv, 'C1', 'new-message', '').engage).toBe(false); + }); + }); + + describe("engageMode='pattern'", () => { + it('pattern="." engages on every source except new-message-with-unknown', () => { + const conv = mapFor(cfg({ engageMode: 'pattern', engagePattern: '.' })); + for (const s of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) { + expect(shouldEngage(conv, 'C1', s, 'anything').engage).toBe(true); + } + }); + + it('tests regex against text on new-message (the main bug fix)', () => { + const conv = mapFor(cfg({ engageMode: 'pattern', engagePattern: '^!report' })); + expect(shouldEngage(conv, 'C1', 'new-message', '!report now').engage).toBe(true); + expect(shouldEngage(conv, 'C1', 'new-message', 'nothing to see').engage).toBe(false); + }); + + it('pattern regex applies on every source (symmetry)', () => { + const conv = mapFor(cfg({ engageMode: 'pattern', engagePattern: 'deploy' })); + for (const s of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) { + expect(shouldEngage(conv, 'C1', s, 'time to deploy').engage).toBe(true); + expect(shouldEngage(conv, 'C1', s, 'weather today').engage).toBe(false); + } + }); + + it('pattern never triggers sticky-subscribe', () => { + const conv = mapFor(cfg({ engageMode: 'pattern', engagePattern: '.' })); + for (const s of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) { + expect(shouldEngage(conv, 'C1', s, 'hi').stickySubscribe).toBe(false); + } + }); + + it('invalid regex fails open (admin sees something rather than silent drop)', () => { + const conv = mapFor(cfg({ engageMode: 'pattern', engagePattern: '[unclosed' })); + expect(shouldEngage(conv, 'C1', 'new-message', 'x').engage).toBe(true); + }); + }); + + describe('multiple wirings on one conversation', () => { + it('takes the union across wirings (any-engage wins)', () => { + // mention wiring + pattern wiring on the same channel. A plain message + // should engage via the pattern wiring even though the mention wiring + // alone would reject it. + const conv = mapFor( + cfg({ agentGroupId: 'ag-a', engageMode: 'mention' }), + cfg({ agentGroupId: 'ag-b', engageMode: 'pattern', engagePattern: '^hi' }), + ); + expect(shouldEngage(conv, 'C1', 'new-message', 'hi there').engage).toBe(true); + expect(shouldEngage(conv, 'C1', 'new-message', 'something else').engage).toBe(false); + }); + + it('stickySubscribe from any mention-sticky wiring wins', () => { + const conv = mapFor( + cfg({ agentGroupId: 'ag-a', engageMode: 'mention' }), + cfg({ agentGroupId: 'ag-b', engageMode: 'mention-sticky' }), + ); + expect(shouldEngage(conv, 'C1', 'mention', '')).toEqual({ engage: true, stickySubscribe: true }); + }); + }); +}); diff --git a/src/channels/chat-sdk-bridge.ts b/src/channels/chat-sdk-bridge.ts index 6a480c4..f2daf11 100644 --- a/src/channels/chat-sdk-bridge.ts +++ b/src/channels/chat-sdk-bridge.ts @@ -65,6 +65,99 @@ export interface ChatSdkBridgeConfig { transformOutboundText?: (text: string) => string; } +/** + * Which Chat SDK handler delivered this message. Determines which engage modes + * can fire. + * + * - `subscribed` — `onSubscribedMessage`. Thread is already subscribed. + * Every wiring mode (mention / mention-sticky / pattern) + * evaluates normally. + * - `mention` — `onNewMention`. Bot was @-mentioned in an unsubscribed + * thread. mention + mention-sticky engage; pattern runs + * the regex. + * - `dm` — `onDirectMessage`. Unsubscribed DM. Treated like a + * mention for engagement purposes. + * - `new-message` — `onNewMessage(/./, …)`. Plain non-mention non-DM + * message in an unsubscribed thread. Only `pattern` + * wirings can fire here. mention / mention-sticky ignore + * this source (they require an explicit mention). + */ +export type EngageSource = 'subscribed' | 'mention' | 'dm' | 'new-message'; + +/** + * Should a message from (channelId, source, text) engage any of the wired + * agents on this conversation? + * + * Exported for testability — see `chat-sdk-bridge.test.ts`. + * + * We take the union across wired agents: if any wiring would engage, the + * message is forwarded. Per-agent filtering after that happens in the host + * router (see `src/router.ts` pickAgents). + */ +export function shouldEngage( + conversations: Map, + channelId: string, + source: EngageSource, + text: string, +): { engage: boolean; stickySubscribe: boolean } { + const configs = conversations.get(channelId); + + // Unknown conversation — behavior diverges by source: + // - subscribed/mention/dm: forward anyway. These paths imply some + // prior engagement (subscribe, @mention, DM open) and may be a new + // group that hasn't been registered yet; central routing will log + + // drop cleanly. + // - new-message: DROP. `onNewMessage(/./, …)` fires for every message + // in every unsubscribed thread the bot can see — including channels + // the bot is merely *present* in but not wired to. Forwarding + // everything would flood the host. + if (!configs || configs.length === 0) { + return { engage: source !== 'new-message', stickySubscribe: false }; + } + + let engage = false; + let stickySubscribe = false; + + for (const cfg of configs) { + switch (cfg.engageMode) { + case 'mention': + if (source === 'mention' || source === 'dm') engage = true; + break; + case 'mention-sticky': + if (source === 'mention' || source === 'dm') { + engage = true; + stickySubscribe = true; + } else if (source === 'subscribed') { + // Thread was already subscribed on a prior mention — treat as + // engage-all so follow-ups in the thread reach the agent. + engage = true; + } + // source='new-message' → do not engage. mention-sticky requires an + // explicit mention to start the conversation. + break; + case 'pattern': { + // Pattern evaluates on any source that delivers a plain message — + // including new-message, which is the whole reason we registered + // onNewMessage(/./). For mention/dm-delivered messages we still + // test the regex (historical behavior), so pattern='foo' wirings + // only fire on mentions whose text contains 'foo'. + const pattern = cfg.engagePattern ?? '.'; + try { + if (pattern === '.' || new RegExp(pattern).test(text)) engage = true; + } catch { + // Invalid regex → fail open so the admin can see something is + // happening and fix the pattern. + engage = true; + } + break; + } + } + if (engage && stickySubscribe) break; + } + + return { engage, stickySubscribe }; +} + export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter { const { adapter } = config; const transformText = (t: string): string => (config.transformOutboundText ? config.transformOutboundText(t) : t); @@ -92,66 +185,12 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter return map; } - /** - * Should a message from (channelId, kind) engage any of the wired agents? - * - * - `mention` — engages only when the message actually @-mentions - * the bot (the bridge already sees it here because - * Chat SDK only forwards subscribed / mentioned / - * DM messages) - * - `mention-sticky` — same as `mention` for gating, PLUS we subscribe - * the thread so later messages arrive via the - * subscribed path and fall through to an - * engage-all style treatment - * - `pattern` — regex test against message text; `.` = always - * - * We take the union across wired agents — if any one of them would engage, - * the message goes through. Per-agent filtering after that happens in the - * host router (see src/router.ts pickAgents). - */ - function shouldEngage( + function engageDecision( channelId: string, - source: 'subscribed' | 'mention' | 'dm', + source: EngageSource, text: string, ): { engage: boolean; stickySubscribe: boolean } { - const configs = conversations.get(channelId); - // Unknown conversation — forward anyway (may be a new group that - // hasn't been registered yet; central routing will log + drop cleanly). - if (!configs || configs.length === 0) return { engage: true, stickySubscribe: false }; - - let engage = false; - let stickySubscribe = false; - - for (const cfg of configs) { - switch (cfg.engageMode) { - case 'mention': - if (source === 'mention' || source === 'dm') engage = true; - break; - case 'mention-sticky': - if (source === 'mention' || source === 'dm') { - engage = true; - stickySubscribe = true; - } else if (source === 'subscribed') { - // Thread was already subscribed on a prior mention — treat as - // engage-all so follow-ups in the thread reach the agent. - engage = true; - } - break; - case 'pattern': { - const pattern = cfg.engagePattern ?? '.'; - try { - if (pattern === '.' || new RegExp(pattern).test(text)) engage = true; - } catch { - // Invalid regex → fail open so the admin can see something and fix. - engage = true; - } - break; - } - } - if (engage && stickySubscribe) break; - } - - return { engage, stickySubscribe }; + return shouldEngage(conversations, channelId, source, text); } async function messageToInbound(message: ChatMessage): Promise { @@ -238,7 +277,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter chat.onSubscribedMessage(async (thread, message) => { const channelId = adapter.channelIdFromThreadId(thread.id); const text = typeof message.text === 'string' ? message.text : ''; - const decision = shouldEngage(channelId, 'subscribed', text); + const decision = engageDecision(channelId, 'subscribed', text); if (!decision.engage) return; await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message)); }); @@ -248,7 +287,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter chat.onNewMention(async (thread, message) => { const channelId = adapter.channelIdFromThreadId(thread.id); const text = typeof message.text === 'string' ? message.text : ''; - const decision = shouldEngage(channelId, 'mention', text); + const decision = engageDecision(channelId, 'mention', text); if (!decision.engage) return; await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message)); if (decision.stickySubscribe) { @@ -267,7 +306,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter chat.onDirectMessage(async (thread, message) => { const channelId = adapter.channelIdFromThreadId(thread.id); const text = typeof message.text === 'string' ? message.text : ''; - const decision = shouldEngage(channelId, 'dm', text); + const decision = engageDecision(channelId, 'dm', text); log.info('Inbound DM received', { adapter: adapter.name, channelId, @@ -282,6 +321,28 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter } }); + // Plain (non-mention, non-DM) messages in unsubscribed threads. + // + // Chat SDK dispatch (handling-events.mdx §"Handler dispatch order"): + // subscribed threads → onSubscribedMessage; unsubscribed + mention → + // onNewMention; unsubscribed + pattern match → onNewMessage. Dispatch + // is exclusive — at most one handler fires per message. + // + // Without this handler, `engage_mode='pattern'` is silently dropped in + // unsubscribed group threads because the SDK never surfaces the + // message anywhere else. Registering with `/./` lets every wired + // conversation's regex be evaluated in our `shouldEngage` — unknown + // conversations are dropped there (see the source='new-message' + // branch) so this doesn't flood the host on channels the bot isn't + // wired to. + chat.onNewMessage(/./, async (thread, message) => { + const channelId = adapter.channelIdFromThreadId(thread.id); + const text = typeof message.text === 'string' ? message.text : ''; + const decision = engageDecision(channelId, 'new-message', text); + if (!decision.engage) return; + await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message)); + }); + // Handle button clicks (ask_user_question) chat.onAction(async (event) => { if (!event.actionId.startsWith('ncq:')) return;