mirror of
https://github.com/qwibitai/nanoclaw.git
synced 2026-06-12 18:11:51 +08:00
fix(channels): wire accumulate mode through the bridge
The router + session DB were already fully plumbed for
ignored_message_policy='accumulate' — fan-out in routeInbound calls
deliverToAgent(wake=false) for non-engaging agents on accumulate wirings,
writeSessionMessage writes trigger=0, countDueMessages filters trigger=1,
container formatter includes all messages regardless of trigger. But the
Chat SDK bridge dropped non-engaging messages before the router ever saw
them, so accumulate was dead on arrival for every adapter that goes
through the bridge.
Expose ignored_message_policy on ConversationConfig, project it in
buildConversationConfigs, and widen shouldEngage's "forward" decision to
"engage OR accumulate" with the union taken across all wirings on a
conversation. stickySubscribe stays gated on a real engage — subscribing
a thread we'd only silently accumulate on would misrepresent the bot's
presence.
shouldEngage return shape is now { forward, stickySubscribe } — engage
was an internal concept the caller never needed, and conflating it with
forward was the source of this bug.
7 new tests cover: non-engaging messages forwarding under accumulate,
mixed drop/accumulate wirings taking the union, accumulate not
triggering sticky subscribe, unknown-conversation drop precedence over
accumulate, and drop policy preserving existing behavior on engaging
messages.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -22,6 +22,20 @@ export interface ConversationConfig {
|
||||
engageMode: 'pattern' | 'mention' | 'mention-sticky';
|
||||
/** Regex source when engageMode='pattern'. '.' is the "always" sentinel. */
|
||||
engagePattern?: string | null;
|
||||
/**
|
||||
* What to do with messages this wiring doesn't engage on.
|
||||
*
|
||||
* 'drop' — discard silently
|
||||
* 'accumulate' — still forward to the host so the router can store the
|
||||
* message in this agent's session with trigger=0. It
|
||||
* rides along as context when the agent next wakes, but
|
||||
* doesn't wake it on its own.
|
||||
*
|
||||
* The bridge reads this to decide whether to forward a non-engaging
|
||||
* message at all — if any wiring on a conversation has 'accumulate', the
|
||||
* bridge forwards and lets the router apply the per-wiring decision.
|
||||
*/
|
||||
ignoredMessagePolicy?: 'drop' | 'accumulate';
|
||||
sessionMode: 'shared' | 'per-thread' | 'agent-shared';
|
||||
}
|
||||
|
||||
|
||||
@@ -17,6 +17,7 @@ function cfg(
|
||||
agentGroupId: partial.agentGroupId ?? 'ag-1',
|
||||
engageMode: partial.engageMode,
|
||||
engagePattern: partial.engagePattern ?? null,
|
||||
ignoredMessagePolicy: partial.ignoredMessagePolicy ?? 'drop',
|
||||
sessionMode: partial.sessionMode ?? 'shared',
|
||||
};
|
||||
}
|
||||
@@ -66,26 +67,26 @@ describe('shouldEngage', () => {
|
||||
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 });
|
||||
expect(shouldEngage(empty, 'C1', source, '')).toEqual({ forward: true, stickySubscribe: false });
|
||||
});
|
||||
}
|
||||
it("DROPS for source='new-message' (would flood from unwired channels)", () => {
|
||||
expect(shouldEngage(empty, 'C1', 'new-message', 'hello')).toEqual({
|
||||
engage: false,
|
||||
forward: false,
|
||||
stickySubscribe: false,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("engageMode='mention'", () => {
|
||||
describe("engageMode='mention' + ignoredMessagePolicy='drop' (default)", () => {
|
||||
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('forwards on mention + dm', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'mention', '').forward).toBe(true);
|
||||
expect(shouldEngage(conv, 'C1', 'dm', '').forward).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('does NOT forward on subscribed or new-message (prevents keep-firing + flooding)', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'subscribed', '').forward).toBe(false);
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', '').forward).toBe(false);
|
||||
});
|
||||
it('never asks to subscribe', () => {
|
||||
for (const s of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) {
|
||||
@@ -96,37 +97,37 @@ describe('shouldEngage', () => {
|
||||
|
||||
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('forwards on mention + dm with stickySubscribe=true', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'mention', '')).toEqual({ forward: true, stickySubscribe: true });
|
||||
expect(shouldEngage(conv, 'C1', 'dm', '')).toEqual({ forward: true, stickySubscribe: true });
|
||||
});
|
||||
it('engages on subscribed follow-ups without re-subscribing', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'subscribed', '')).toEqual({ engage: true, stickySubscribe: false });
|
||||
it('forwards subscribed follow-ups without re-subscribing', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'subscribed', '')).toEqual({ forward: true, stickySubscribe: false });
|
||||
});
|
||||
it('does NOT engage on new-message (explicit mention required to start)', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', '').engage).toBe(false);
|
||||
it('does NOT forward on new-message (explicit mention required to start)', () => {
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', '').forward).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("engageMode='pattern'", () => {
|
||||
it('pattern="." engages on every source except new-message-with-unknown', () => {
|
||||
it('pattern="." forwards on every source (when conversation is known)', () => {
|
||||
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);
|
||||
expect(shouldEngage(conv, 'C1', s, 'anything').forward).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);
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', '!report now').forward).toBe(true);
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'nothing to see').forward).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);
|
||||
expect(shouldEngage(conv, 'C1', s, 'time to deploy').forward).toBe(true);
|
||||
expect(shouldEngage(conv, 'C1', s, 'weather today').forward).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -139,7 +140,50 @@ describe('shouldEngage', () => {
|
||||
|
||||
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);
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'x').forward).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ignoredMessagePolicy='accumulate'", () => {
|
||||
it('forwards non-engaging new-message so the router can store it as context (trigger=0)', () => {
|
||||
const conv = mapFor(cfg({ engageMode: 'mention', ignoredMessagePolicy: 'accumulate' }));
|
||||
// Plain message in unsubscribed group — mention rule says no engage,
|
||||
// but accumulate says forward anyway.
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'chit chat')).toEqual({
|
||||
forward: true,
|
||||
stickySubscribe: false,
|
||||
});
|
||||
});
|
||||
|
||||
it('forwards non-engaging subscribed messages for accumulation', () => {
|
||||
// mention wiring in a subscribed thread: the mention handler already
|
||||
// fired once, thread is now subscribed, follow-ups route here. The
|
||||
// base 'mention' rule wouldn't engage without an @-mention, but
|
||||
// accumulate says capture the context.
|
||||
const conv = mapFor(cfg({ engageMode: 'mention', ignoredMessagePolicy: 'accumulate' }));
|
||||
expect(shouldEngage(conv, 'C1', 'subscribed', 'follow up talk').forward).toBe(true);
|
||||
});
|
||||
|
||||
it('does NOT set stickySubscribe purely from accumulate (avoid misleading bot presence)', () => {
|
||||
const conv = mapFor(cfg({ engageMode: 'mention-sticky', ignoredMessagePolicy: 'accumulate' }));
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'plain').stickySubscribe).toBe(false);
|
||||
});
|
||||
|
||||
it("accumulate doesn't override the 'unknown conversation → drop new-message' rule", () => {
|
||||
// Unknown conversation (not in map): accumulate can't be read because
|
||||
// there's no config to read from. We still drop.
|
||||
const empty = new Map<string, ConversationConfig[]>();
|
||||
expect(shouldEngage(empty, 'C-unknown', 'new-message', 'x').forward).toBe(false);
|
||||
});
|
||||
|
||||
it("drop policy + non-engaging message → doesn't forward", () => {
|
||||
const conv = mapFor(cfg({ engageMode: 'mention', ignoredMessagePolicy: 'drop' }));
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'plain').forward).toBe(false);
|
||||
});
|
||||
|
||||
it('engaging message with drop policy still forwards (engage wins regardless)', () => {
|
||||
const conv = mapFor(cfg({ engageMode: 'mention', ignoredMessagePolicy: 'drop' }));
|
||||
expect(shouldEngage(conv, 'C1', 'mention', '@bot hi').forward).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -152,8 +196,17 @@ describe('shouldEngage', () => {
|
||||
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);
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'hi there').forward).toBe(true);
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'something else').forward).toBe(false);
|
||||
});
|
||||
|
||||
it('any accumulate wiring causes forward even if all others would drop', () => {
|
||||
const conv = mapFor(
|
||||
cfg({ agentGroupId: 'ag-a', engageMode: 'mention', ignoredMessagePolicy: 'drop' }),
|
||||
cfg({ agentGroupId: 'ag-b', engageMode: 'mention', ignoredMessagePolicy: 'accumulate' }),
|
||||
);
|
||||
// Plain message: ag-a would drop, ag-b would accumulate → forward.
|
||||
expect(shouldEngage(conv, 'C1', 'new-message', 'plain').forward).toBe(true);
|
||||
});
|
||||
|
||||
it('stickySubscribe from any mention-sticky wiring wins', () => {
|
||||
@@ -161,7 +214,7 @@ describe('shouldEngage', () => {
|
||||
cfg({ agentGroupId: 'ag-a', engageMode: 'mention' }),
|
||||
cfg({ agentGroupId: 'ag-b', engageMode: 'mention-sticky' }),
|
||||
);
|
||||
expect(shouldEngage(conv, 'C1', 'mention', '')).toEqual({ engage: true, stickySubscribe: true });
|
||||
expect(shouldEngage(conv, 'C1', 'mention', '')).toEqual({ forward: true, stickySubscribe: true });
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -85,21 +85,28 @@ export interface ChatSdkBridgeConfig {
|
||||
export type EngageSource = 'subscribed' | 'mention' | 'dm' | 'new-message';
|
||||
|
||||
/**
|
||||
* Should a message from (channelId, source, text) engage any of the wired
|
||||
* agents on this conversation?
|
||||
* Should a message from (channelId, source, text) be forwarded to the host,
|
||||
* and if so, should the bridge subscribe the thread?
|
||||
*
|
||||
* 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).
|
||||
* We take the union across wired agents: if any wiring would engage OR any
|
||||
* wiring has `ignoredMessagePolicy='accumulate'`, the message is forwarded.
|
||||
* The host router then does the per-wiring decision in `deliverToAgent` —
|
||||
* engaging agents get `trigger=1` (wake), accumulating agents get
|
||||
* `trigger=0` (store as context, don't wake), drop-policy agents are
|
||||
* skipped (see `src/router.ts` routeInbound fan-out).
|
||||
*
|
||||
* `stickySubscribe` is only set when an actual engage happens (not just
|
||||
* accumulate) — subscribing a thread we'd only silently accumulate on would
|
||||
* misrepresent the bot's presence to other users.
|
||||
*/
|
||||
export function shouldEngage(
|
||||
conversations: Map<string, ConversationConfig[]>,
|
||||
channelId: string,
|
||||
source: EngageSource,
|
||||
text: string,
|
||||
): { engage: boolean; stickySubscribe: boolean } {
|
||||
): { forward: boolean; stickySubscribe: boolean } {
|
||||
const configs = conversations.get(channelId);
|
||||
|
||||
// Unknown conversation — behavior diverges by source:
|
||||
@@ -112,28 +119,30 @@ export function shouldEngage(
|
||||
// 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 };
|
||||
return { forward: source !== 'new-message', stickySubscribe: false };
|
||||
}
|
||||
|
||||
let engage = false;
|
||||
let accumulate = false;
|
||||
let stickySubscribe = false;
|
||||
|
||||
for (const cfg of configs) {
|
||||
let cfgEngages = false;
|
||||
switch (cfg.engageMode) {
|
||||
case 'mention':
|
||||
if (source === 'mention' || source === 'dm') engage = true;
|
||||
if (source === 'mention' || source === 'dm') cfgEngages = true;
|
||||
break;
|
||||
case 'mention-sticky':
|
||||
if (source === 'mention' || source === 'dm') {
|
||||
engage = true;
|
||||
cfgEngages = 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;
|
||||
cfgEngages = true;
|
||||
}
|
||||
// source='new-message' → do not engage. mention-sticky requires an
|
||||
// explicit mention to start the conversation.
|
||||
// source='new-message' → does not engage (requires explicit mention
|
||||
// to start). Accumulate policy is evaluated below if set.
|
||||
break;
|
||||
case 'pattern': {
|
||||
// Pattern evaluates on any source that delivers a plain message —
|
||||
@@ -143,19 +152,27 @@ export function shouldEngage(
|
||||
// only fire on mentions whose text contains 'foo'.
|
||||
const pattern = cfg.engagePattern ?? '.';
|
||||
try {
|
||||
if (pattern === '.' || new RegExp(pattern).test(text)) engage = true;
|
||||
if (pattern === '.' || new RegExp(pattern).test(text)) cfgEngages = true;
|
||||
} catch {
|
||||
// Invalid regex → fail open so the admin can see something is
|
||||
// happening and fix the pattern.
|
||||
engage = true;
|
||||
cfgEngages = true;
|
||||
}
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (engage && stickySubscribe) break;
|
||||
|
||||
if (cfgEngages) {
|
||||
engage = true;
|
||||
} else if (cfg.ignoredMessagePolicy === 'accumulate') {
|
||||
// Wiring doesn't engage on this message but wants it captured as
|
||||
// context for its session — forward so the router can write it with
|
||||
// trigger=0.
|
||||
accumulate = true;
|
||||
}
|
||||
}
|
||||
|
||||
return { engage, stickySubscribe };
|
||||
return { forward: engage || accumulate, stickySubscribe };
|
||||
}
|
||||
|
||||
export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter {
|
||||
@@ -189,7 +206,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter
|
||||
channelId: string,
|
||||
source: EngageSource,
|
||||
text: string,
|
||||
): { engage: boolean; stickySubscribe: boolean } {
|
||||
): { forward: boolean; stickySubscribe: boolean } {
|
||||
return shouldEngage(conversations, channelId, source, text);
|
||||
}
|
||||
|
||||
@@ -278,7 +295,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter
|
||||
const channelId = adapter.channelIdFromThreadId(thread.id);
|
||||
const text = typeof message.text === 'string' ? message.text : '';
|
||||
const decision = engageDecision(channelId, 'subscribed', text);
|
||||
if (!decision.engage) return;
|
||||
if (!decision.forward) return;
|
||||
await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message));
|
||||
});
|
||||
|
||||
@@ -288,7 +305,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter
|
||||
const channelId = adapter.channelIdFromThreadId(thread.id);
|
||||
const text = typeof message.text === 'string' ? message.text : '';
|
||||
const decision = engageDecision(channelId, 'mention', text);
|
||||
if (!decision.engage) return;
|
||||
if (!decision.forward) return;
|
||||
await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message));
|
||||
if (decision.stickySubscribe) {
|
||||
await thread.subscribe();
|
||||
@@ -312,9 +329,9 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter
|
||||
channelId,
|
||||
sender: (message.author as any)?.fullName ?? (message.author as any)?.userId ?? 'unknown',
|
||||
threadId: thread.id,
|
||||
engage: decision.engage,
|
||||
forward: decision.forward,
|
||||
});
|
||||
if (!decision.engage) return;
|
||||
if (!decision.forward) return;
|
||||
await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message));
|
||||
if (decision.stickySubscribe) {
|
||||
await thread.subscribe();
|
||||
@@ -339,7 +356,7 @@ export function createChatSdkBridge(config: ChatSdkBridgeConfig): ChannelAdapter
|
||||
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;
|
||||
if (!decision.forward) return;
|
||||
await setupConfig.onInbound(channelId, thread.id, await messageToInbound(message));
|
||||
});
|
||||
|
||||
|
||||
@@ -163,6 +163,7 @@ function buildConversationConfigs(channelType: string): ConversationConfig[] {
|
||||
agentGroupId: agent.agent_group_id,
|
||||
engageMode: agent.engage_mode,
|
||||
engagePattern: agent.engage_pattern,
|
||||
ignoredMessagePolicy: agent.ignored_message_policy,
|
||||
sessionMode: agent.session_mode,
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user