wangjunxiao/nanoclaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(channels): shrink bridge shouldEngage to flood gate + subscribe signal

Browse Source 
Before this change the bridge and the router both owned engage_mode
policy. Bridge's shouldEngage had a full switch over mention /
mention-sticky / pattern + source-based rules + engage_pattern regex
test + ignored_message_policy accumulate fallback. Router's
evaluateEngage had the same switch against the same fields. Two
parallel logic paths with subtle vocabulary differences (bridge: "which
SDK handler fired"; router: "what isMention says"). Every time we
touched one we had to reason about the other — the Telegram
hasMention bug and the "pattern mode silently drops in group chats"
bug were both drift between the two.

Refactor to one place. Router keeps all per-wiring policy — engage
mode, pattern regex, sender scope, ignored-message policy — unchanged.
Bridge drops to a coarse flood gate + subscribe signal:

  - forward: does this channel have ANY wiring? Forward if yes.
    Unknown channels still forward for subscribed/mention/dm (they may
    be newly auto-created, or will trigger the coming
    channel-registration flow). Unknown channels DROP for new-message
    so we don't flood from every unsubscribed thread the bot happens
    to sit in.

  - stickySubscribe: any mention-sticky wiring on the channel AND the
    source is mention or dm. Coarse union — subscribe is idempotent
    and one call serves every sticky wiring.

The `text` param on shouldEngage is gone (pattern regex lives in the
router now). Four bridge handler sites simplify accordingly. messageToInbound
still carries the SDK-confirmed isMention flag through to the router
unchanged.

Behavioral delta: pure-mention-wired channels (no pattern, no
accumulate) will now see every plain group message reach the router
before being dropped there, where before the bridge dropped at the
transport boundary. Extra DB lookup per dropped message in this
specific case; acceptable for the cleaner seam and can be optimized
back at the bridge if it ever matters in practice.

Bridge tests prune the 10 engage_mode-specific cases that covered
logic now owned by evaluateEngage in the router (host-core.test.ts
covers it end-to-end through routeInbound). Bridge tests keep only
what's bridge-specific: the flood gate and the stickySubscribe
coarse union.

172 tests pass (was 182 — net -10 redundant bridge tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gavrielc
2026-04-20 13:32:08 +03:00
parent 68058cbc4a
commit b15972284b
2 changed files with 98 additions and 254 deletions
Download Patch File Download Diff File Expand all files Collapse all files

180
src/channels/chat-sdk-bridge.test.ts
View File

@@ -61,160 +61,74 @@ describe('createChatSdkBridge', () => {
});
});
describe('shouldEngage', () => {
describe('unknown conversation', () => {
describe('shouldEngage (bridge-level flood gate + subscribe signal)', () => {
// Per-wiring engage_mode / engage_pattern / ignored_message_policy
// semantics live in the router (evaluateEngage / routeInbound fan-out).
// These tests only cover the bridge's two responsibilities: should we
// forward at all, and should we call thread.subscribe().
describe('flood gate — unknown conversation', () => {
const empty = new Map<string, ConversationConfig[]>();
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({ forward: true, stickySubscribe: false });
const carriedSources: EngageSource[] = ['subscribed', 'mention', 'dm'];
for (const source of carriedSources) {
it(`forwards for source='${source}' (may be a newly-auto-created channel or a channel-registration trigger)`, () => {
expect(shouldEngage(empty, 'C-new', 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({
it("DROPS for source='new-message' (onNewMessage(/./) fires for every unsubscribed thread the bot can see — would flood)", () => {
expect(shouldEngage(empty, 'C-unwired', 'new-message')).toEqual({
forward: false,
stickySubscribe: false,
});
});
});
describe("engageMode='mention' + ignoredMessagePolicy='drop' (default)", () => {
describe('known conversation — bridge forwards regardless of engage mode', () => {
// Policy lives in the router now. The bridge only knows "has any wiring".
const conv = mapFor(cfg({ engageMode: 'mention' }));
it('forwards on mention + dm', () => {
expect(shouldEngage(conv, 'C1', 'mention', '').forward).toBe(true);
expect(shouldEngage(conv, 'C1', 'dm', '').forward).toBe(true);
});
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[]) {
expect(shouldEngage(conv, 'C1', s, '').stickySubscribe).toBe(false);
}
});
});
describe("engageMode='mention-sticky'", () => {
const conv = mapFor(cfg({ engageMode: 'mention-sticky' }));
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('forwards subscribed follow-ups without re-subscribing', () => {
expect(shouldEngage(conv, 'C1', 'subscribed', '')).toEqual({ forward: true, stickySubscribe: 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="." 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').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').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').forward).toBe(true);
expect(shouldEngage(conv, 'C1', s, 'weather today').forward).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').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,
for (const source of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) {
it(`forwards for source='${source}' — router will decide engage / accumulate / drop per wiring`, () => {
expect(shouldEngage(conv, 'C1', source).forward).toBe(true);
});
});
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);
});
}
});
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').forward).toBe(true);
expect(shouldEngage(conv, 'C1', 'new-message', 'something else').forward).toBe(false);
describe('stickySubscribe signal', () => {
it('true when any mention-sticky wiring exists AND source is mention', () => {
const conv = mapFor(cfg({ engageMode: 'mention-sticky' }));
expect(shouldEngage(conv, 'C1', 'mention').stickySubscribe).toBe(true);
});
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('true when any mention-sticky wiring exists AND source is dm', () => {
const conv = mapFor(cfg({ engageMode: 'mention-sticky' }));
expect(shouldEngage(conv, 'C1', 'dm').stickySubscribe).toBe(true);
});
it('stickySubscribe from any mention-sticky wiring wins', () => {
it('false on subscribed — thread is already subscribed, no need to re-subscribe', () => {
const conv = mapFor(cfg({ engageMode: 'mention-sticky' }));
expect(shouldEngage(conv, 'C1', 'subscribed').stickySubscribe).toBe(false);
});
it('false on new-message — mention-sticky requires an explicit mention to start', () => {
const conv = mapFor(cfg({ engageMode: 'mention-sticky' }));
expect(shouldEngage(conv, 'C1', 'new-message').stickySubscribe).toBe(false);
});
it('false for plain mention / pattern wirings (not sticky)', () => {
const mentionConv = mapFor(cfg({ engageMode: 'mention' }));
const patternConv = mapFor(cfg({ engageMode: 'pattern', engagePattern: '.' }));
for (const s of ['subscribed', 'mention', 'dm', 'new-message'] as EngageSource[]) {
expect(shouldEngage(mentionConv, 'C1', s).stickySubscribe).toBe(false);
expect(shouldEngage(patternConv, 'C1', s).stickySubscribe).toBe(false);
}
});
it('fires on coarse union — mixed wirings where any one is mention-sticky', () => {
const conv = mapFor(
cfg({ agentGroupId: 'ag-a', engageMode: 'mention' }),
cfg({ agentGroupId: 'ag-b', engageMode: 'mention-sticky' }),
);
expect(shouldEngage(conv, 'C1', 'mention', '')).toEqual({ forward: true, stickySubscribe: true });
expect(shouldEngage(conv, 'C1', 'mention').stickySubscribe).toBe(true);
});
});
});