From 4150a5e7f1d304014805ae562ccfcb155cab9827 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:18:21 -0700 Subject: [PATCH] test(mentions): pin the mechanism behind "addressing is never scoped by the thread" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's gate on #1216 named the gap: the tests there pin the CUE TEXT, and the sentence the cue teaches would become a lie the moment someone added one narrowToThread call to the mention fan-out — with all 129 copy assertions green. Copy is not mechanism. This asserts the call graph instead. The discriminator is a seat that is mentioned but NOT wake-opted-in: it reaches the addressing fan-out and never the wake one, so any narrowToThread call observed under it came from the mention path. Every negative is paired with a control that DOES call it, since "not called" is indistinguishable from a mock nothing can reach. Demonstrated rather than asserted. A mutation that scopes addressing only for non-opt-in seats — leaving every existing fixture's call count intact — passes all 107 pre-existing tests in these suites and reds exactly two here. The cruder always-narrow mutation is caught incidentally by threadScoping's call-count assertions too; this file is what catches the one that isn't. Also corrects a comment the first draft of this test believed. agentMention Service:1123 said the scoping branch "runs only when !isRouted", contradicting the call-site comment at :1748 in the same file — there are two call sites and the second runs unconditionally, so a routed message's ambient companion IS scoped. Addressing survives because the chat.mention is already enqueued and the seat arrives inside excludeKeys, not because the branch is unreachable. Right conclusion, false mechanism, which is the kind of comment that makes the next reader's test wrong. Co-Authored-By: Claude Opus 5 --- ...ervice.addressingIsNotThreadScoped.test.js | 221 ++++++++++++++++++ backend/services/agentMentionService.ts | 13 +- 2 files changed, 231 insertions(+), 3 deletions(-) create mode 100644 backend/__tests__/unit/services/agentMentionService.addressingIsNotThreadScoped.test.js diff --git a/backend/__tests__/unit/services/agentMentionService.addressingIsNotThreadScoped.test.js b/backend/__tests__/unit/services/agentMentionService.addressingIsNotThreadScoped.test.js new file mode 100644 index 000000000..1362164d7 --- /dev/null +++ b/backend/__tests__/unit/services/agentMentionService.addressingIsNotThreadScoped.test.js @@ -0,0 +1,221 @@ +/** + * Addressing is never scoped by the thread — the mechanism, not the copy. + * + * `threadWakeScopeService`'s header states it in prose ("NOT the addressing + * path"), and #1216's wake cue teaches agents to rely on it. @sprint-review's + * gate on that PR named the gap exactly: the tests there pin the CUE TEXT, and + * "addressing is never scoped by the thread" would become a lie the moment + * someone added one `narrowToThread` call to the mention fan-out — with every + * copy assertion still green, because copy is not mechanism. + * + * This file is that missing pin. It asserts the call graph rather than the + * sentence: `narrowToThread` runs on the WAKE fan-out and never on the + * addressing fan-out, so a mute or a non-follow can silence ambient activity + * and can never silence an @mention. + * + * The discriminator is a seat that is mentioned but NOT wake-opted-in. That + * seat reaches the addressing fan-out and never the wake one, so any + * `narrowToThread` call observed under it came from the mention path — the + * mutation this file exists to catch. The paired control is an unrouted + * threaded message to an opted-in seat, which MUST call it: without that, + * `not.toHaveBeenCalled()` would pass just as well from a mock nothing can + * reach, which is the failure mode the sibling suite + * (`threadingIsNotAddressing`) already had to learn once. + */ + +jest.mock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn(), findOne: jest.fn() }, +})); +jest.mock('../../../models/AgentProfile', () => ({ find: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ findById: jest.fn(), find: jest.fn() })); +jest.mock('../../../models/User', () => ({ find: jest.fn(), findById: jest.fn() })); +jest.mock('../../../services/chatSummarizerService', () => ({ + constructor: { getLatestPodSummary: jest.fn() }, + summarizePodMessages: jest.fn(), +})); +jest.mock('../../../models/AgentEvent', () => ({ countDocuments: jest.fn() })); +jest.mock('../../../services/welcomeWakeService', () => ({ maybeFireWelcomeWake: jest.fn() })); +jest.mock('../../../models/pg/Message', () => ({ + findById: jest.fn(async (id) => (String(id) === '101' ? { id: 101, user_id: 'bot-1' } : null)), +})); +jest.mock('../../../models/pg/ThreadUserState', () => ({ + followByParticipation: jest.fn().mockResolvedValue(true), +})); + +// The instrument. Identity by default — narrowing that removes nothing — so +// the observable under test is WHETHER it was consulted, not what it returned. +// A mock that dropped targets would conflate "addressing was scoped" with +// "addressing was suppressed", and only the first is the claim here. +jest.mock('../../../services/threadWakeScopeService', () => ({ + narrowToThread: jest.fn(async (_rootId, targets) => targets), +})); + +const AgentMentionService = require('../../../services/agentMentionService'); +const AgentEventService = require('../../../services/agentEventService'); +const { AgentInstallation } = require('../../../models/AgentRegistry'); +const AgentProfile = require('../../../models/AgentProfile'); +const Pod = require('../../../models/Pod'); +const User = require('../../../models/User'); +const AgentEvent = require('../../../models/AgentEvent'); +const { narrowToThread } = require('../../../services/threadWakeScopeService'); + +const SEAT = { agentName: 'seat-a', instanceId: 'default', displayName: 'Seat A' }; +const SEAT_OPTED_IN = { ...SEAT, config: { wakeOnMessage: { enabled: true } } }; + +const mockInstallations = (installations) => { + AgentInstallation.find.mockReturnValue({ lean: jest.fn().mockResolvedValue(installations) }); + AgentProfile.find.mockReturnValue({ lean: jest.fn().mockResolvedValue([]) }); +}; + +const mockUsers = () => { + User.findById.mockImplementation((id) => ({ + select: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue( + String(id) === 'bot-1' + ? { _id: 'bot-1', isBot: true, botMetadata: { agentName: 'seat-a', instanceId: 'default' } } + : { _id: 'user-1', isBot: false }, + ), + })); + User.find.mockReturnValue({ select: jest.fn().mockReturnThis(), lean: jest.fn().mockResolvedValue([]) }); +}; + +const byType = (t) => AgentEventService.enqueue.mock.calls.map(([a]) => a).filter((a) => a.type === t); +const mentions = () => byType('chat.mention'); +const wakes = () => byType('message.posted'); + +const send = (message) => AgentMentionService.enqueueMentions({ + podId: 'pod-1', userId: 'user-1', username: 'alice', ...message, +}); + +beforeEach(() => { + jest.clearAllMocks(); + mockUsers(); + narrowToThread.mockImplementation(async (_rootId, targets) => targets); + Pod.findById.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue({ type: 'chat', members: ['user-1', 'bot-1'] }), + }), + lean: jest.fn().mockResolvedValue({ _id: 'pod-1', type: 'chat', members: ['user-1', 'bot-1'] }), + }); + AgentEvent.countDocuments.mockResolvedValue(0); +}); + +describe('the addressing fan-out never consults thread scope', () => { + test('an @mention inside a thread is delivered without narrowToThread being called', async () => { + // Not opted in to wake-on-message, so the only fan-out this message can + // reach is the addressing one. + mockInstallations([SEAT]); + + await send({ + message: { + id: 'msg-2', + content: '@seat-a what do you think', + thread_root_id: 101, + threadRootId: 101, + }, + }); + + expect(mentions()).toHaveLength(1); + // The load-bearing assertion. Adding `narrowToThread` to the mention path + // reds this line while every copy test about "addressing is never scoped + // by the thread" stays green. + expect(narrowToThread).not.toHaveBeenCalled(); + }); + + test('CONTROL: an unrouted threaded message to an opted-in seat DOES call it', async () => { + // Proves the mock is reachable from production code. Without this, the + // assertion above passes from a mis-pathed jest.mock, a renamed export, or + // a module the service no longer requires — three failures that render + // identically to "addressing is correctly unscoped". + mockInstallations([SEAT_OPTED_IN]); + + await send({ + message: { id: 'msg-3', content: 'thinking out loud in a thread', thread_root_id: 101, threadRootId: 101 }, + }); + + expect(narrowToThread).toHaveBeenCalledTimes(1); + expect(wakes()).toHaveLength(1); + expect(mentions()).toHaveLength(0); + }); + + test('a ROUTED threaded message still has an ambient fan-out, and it IS narrowed', async () => { + // The case where the two fan-outs collide, and the one that corrected this + // file's first draft. I assumed a routed message skips the wake path + // entirely, on the strength of the comment at agentMentionService:1123 + // ("This branch runs only when `!isRouted`"). It does not: there are TWO + // call sites, and the second (:1748) runs unconditionally, after the + // mention path, to reach opt-ins the mention did not. The comment at that + // call site says so outright; the one inside the function contradicts it. + // + // So the invariant is NOT "a routed message never touches thread scope". + // It is the narrower and more useful one: scoping applies to the ambient + // companion, and the chat.mention is delivered either way. + mockInstallations([SEAT_OPTED_IN]); + + await send({ + message: { id: 'msg-4', content: '@seat-a please look', thread_root_id: 101, threadRootId: 101 }, + }); + + expect(mentions()).toHaveLength(1); + expect(narrowToThread).toHaveBeenCalledTimes(1); + // The mentioned seat is excluded from its own ambient companion, so the + // narrowing cannot double-deliver — it can only remove. + expect(wakes()).toHaveLength(0); + }); + + test('a narrowing that drops every target still cannot suppress a mention', async () => { + // The strongest form: even if thread scope says this seat follows nothing, + // the @mention is delivered. A mute scopes ambient activity, never + // addressing — `threadWakeScopeService`'s header claim, executable. + narrowToThread.mockImplementation(async () => []); + mockInstallations([SEAT_OPTED_IN]); + + await send({ + message: { id: 'msg-5', content: '@seat-a urgent', thread_root_id: 101, threadRootId: 101 }, + }); + + expect(mentions()).toHaveLength(1); + }); + + test('CONTROL: the same total narrowing DOES suppress the ambient wake', async () => { + // Pairs with the test above. Without it, "the mention survived" is equally + // consistent with a narrowing mock that never took effect at all. + narrowToThread.mockImplementation(async () => []); + mockInstallations([SEAT_OPTED_IN]); + + await send({ + message: { id: 'msg-6', content: 'ambient thought', thread_root_id: 101, threadRootId: 101 }, + }); + + expect(wakes()).toHaveLength(0); + }); +}); + +describe('the call site itself', () => { + test('narrowToThread has exactly one call site, inside enqueueWakeOnMessage', () => { + // Structural backstop, and the earliest warning: the behavioural tests + // above cover the paths a message can take today, and this catches a + // second call site added on a path no fixture exercises yet. + const fs = require('fs'); + const path = require('path'); + const src = fs.readFileSync( + path.join(__dirname, '../../../services/agentMentionService.ts'), 'utf8', + ); + + const calls = [...src.matchAll(/\bnarrowToThread\s*\(/g)].map((m) => m.index); + expect(calls).toHaveLength(1); + + const wakeDecl = src.indexOf('const enqueueWakeOnMessage = async ({'); + const mentionDecl = src.indexOf('const enqueueMentions = async ({'); + expect(wakeDecl).toBeGreaterThan(-1); + expect(mentionDecl).toBeGreaterThan(-1); + + // Both declarations exist, so "the call is after enqueueWakeOnMessage" + // means something only if we also know where enqueueMentions begins — + // otherwise a call moved into a later function would still pass. + const [callIdx] = calls; + expect(callIdx).toBeGreaterThan(wakeDecl); + if (mentionDecl > wakeDecl) expect(callIdx).toBeLessThan(mentionDecl); + }); +}); diff --git a/backend/services/agentMentionService.ts b/backend/services/agentMentionService.ts index 5f14a0724..1fd018f24 100644 --- a/backend/services/agentMentionService.ts +++ b/backend/services/agentMentionService.ts @@ -1120,9 +1120,16 @@ const enqueueWakeOnMessage = async ({ // - it can only NARROW. A thread follower who never opted into // wake-on-message still gets nothing, because threading is additive and // must not start delivering to seats that opted into nothing. - // - it cannot touch addressing. This branch runs only when `!isRouted`, so - // an @mention or a reply edge has already left via the mention path. A - // mute scopes ambient activity; it never suppresses being addressed. + // - it cannot touch addressing. NOT because this branch is unreachable on + // a routed message — it is reachable, via the second call site at the + // tail of enqueueMentions, which runs unconditionally so a routed + // message's ambient companion is scoped too. It cannot touch addressing + // because the chat.mention has ALREADY been enqueued by the time this + // runs, and the mentioned seat arrives here inside `excludeKeys`. A mute + // scopes ambient activity; it never suppresses being addressed. + // (An earlier version of this comment claimed the branch runs only when + // `!isRouted`, contradicting the call-site comment below it and giving a + // reader the right conclusion from a false mechanism.) // // Keyed on the BOT's User row id, which is what thread_user_state.user_id // holds: the follow/mute routes are dualAuth, so an agent writing its own