From e58a491f8202e7c467bbc4dbc7c0a230f65139bf Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 26 Aug 2026 08:22:15 -0700 Subject: [PATCH] test(agent-msg): pin the three postMessage observability warns behaviourally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces a source-text assertion that read agentMessageService.ts and matched the `let sanitizedContent = ...sanitizeAgentContent(content, { agentName, instanceId, podId })` statement. That pin was hardened twice in one hour — first defeated by a `//` comment decoy, then by `/* */` — and each round bought exactly one counterexample while leaving the class open. It still passed with the feature off if the text lived in a string literal, or in a second, unreachable `let sanitizedContent = …` elsewhere in a 1,900-line class. Any assertion over source text is defeated by any occurrence that does not execute; comments were the likeliest instance, not the last one. The replacement drives `postMessage` against its mock harness and asserts the warn actually fires, with identity, which is the only form that distinguishes wired from textually-resembles-being-wired — and it is blind to nothing a rename can do. Also covers the two sibling suppressions (runtime model-failure, gateway tool-failure note), which had no delivery pin of any kind. Each is a warn immediately before `sanitizedContent = ''`, so deleting the warn and keeping the zeroing loses the entire record of a swallowed post while every predicate test stays green. Each negative is paired with a control, so this cannot decay into "warns on every post": total-match suppression, a backticked sentinel, and ordinary prose must all stay silent. Mutation table, run rather than reasoned (10 suites, 98 tests): - drop `{agentName,instanceId,podId}` at the call site → 1 red ... and 92/92 GREEN with this file excluded, which is the point: nothing else in the repo catches it now that the source pin is gone - delete the model-failure warn → 1 red - delete the tool-failure warn → 1 red Backend typecheck: 50 errors with and without — identical baseline. Co-Authored-By: Claude Opus 5 --- .../agentMessageService.chatNoise.test.js | 39 +--- ...ssageService.observabilityDelivery.test.js | 210 ++++++++++++++++++ 2 files changed, 220 insertions(+), 29 deletions(-) create mode 100644 backend/__tests__/unit/services/agentMessageService.observabilityDelivery.test.js diff --git a/backend/__tests__/unit/services/agentMessageService.chatNoise.test.js b/backend/__tests__/unit/services/agentMessageService.chatNoise.test.js index bbbf32d98..8789ea8fc 100644 --- a/backend/__tests__/unit/services/agentMessageService.chatNoise.test.js +++ b/backend/__tests__/unit/services/agentMessageService.chatNoise.test.js @@ -154,35 +154,16 @@ describe('AgentMessageService.sanitizeAgentContent — strip observability', () expect(stripWarnings()).toHaveLength(0); }); - // Delivery pin, not a behaviour pin — and the distinction is the point. - // Mutating the postMessage call site to drop `{ agentName, instanceId, podId }` - // left every assertion above green: they exercise the sanitizer directly, so - // they pin the predicate and say nothing about whether the posting path ever - // opts in. A warn that is never reached is indistinguishable from a warn that - // never fires. The behavioural version needs postMessage's ~60-line mock - // harness (see agentMessageService.phantom-directive.test.js); this is the - // cheap pin that catches the mutation that actually happened. - it('is wired at the postMessage call site — the opt-in is what makes it fire', () => { - const fs = require('fs'); - const path = require('path'); - const src = fs.readFileSync( - path.join(__dirname, '../../../services/agentMessageService.ts'), - 'utf8', - ); - // Comments are stripped before matching. A bare `toContain` on the call - // text is satisfied by PROSE: delete the argument from the real call and - // leave the old form in a `//` comment above it, and the assertion passes - // with the feature entirely off. Not hypothetical in this file — it - // discusses `sanitizeAgentContent` in comments at :110 and :1126. - const code = src - .replace(/\/\*[\s\S]*?\*\//g, '') - .replace(/(^|[^:])\/\/[^\n]*/g, '$1'); - // Anchored on the assignment, so the match is the statement that feeds - // postMessage rather than any mention of the call anywhere in the file. - expect(code).toMatch( - /let sanitizedContent = AgentMessageService\.sanitizeAgentContent\(\s*content,\s*\{[^}]*agentName[^}]*instanceId[^}]*podId[^}]*\}\s*\)/, - ); - }); + // The delivery pin that used to live here — a regex over the service's + // source text, asserting the `postMessage` call site passes + // `{ agentName, instanceId, podId }` — is GONE, replaced rather than + // hardened a third time. It was defeated by a `//` comment decoy, then by + // `/* */`, and each fix bought one counterexample while leaving the class + // open: any assertion over source text passes on any occurrence that does + // not execute. Its replacement drives `postMessage` and asserts the warn + // actually fires — see agentMessageService.observabilityDelivery.test.js, + // which also covers the two sibling suppressions that never had a delivery + // pin of any kind. it('names the agent, instance and pod, like the two suppressions beside it', () => { AgentMessageService.sanitizeAgentContent('A reply of NO_REPLY means silence.', OBSERVE); diff --git a/backend/__tests__/unit/services/agentMessageService.observabilityDelivery.test.js b/backend/__tests__/unit/services/agentMessageService.observabilityDelivery.test.js new file mode 100644 index 000000000..806a0bb78 --- /dev/null +++ b/backend/__tests__/unit/services/agentMessageService.observabilityDelivery.test.js @@ -0,0 +1,210 @@ +/** + * TASK-075 — behavioural delivery pins for the three observability warns on + * the `postMessage` path. + * + * These REPLACE a source-text assertion in + * `agentMessageService.chatNoise.test.js` that read the service file and + * matched the `let sanitizedContent = ...sanitizeAgentContent(content, {...})` + * statement. That pin was hardened twice in one hour — first defeated by a + * `//` comment decoy, then by `/* *\/` — and each round bought exactly one + * counterexample while leaving the class open: it still passed with the + * feature off if the text lived in a string literal, or in a second, + * unreachable `let sanitizedContent = …` in another method of a 1,900-line + * class. Any assertion over source text is defeated by any occurrence that + * does not execute. Comments were the likeliest instance, not the last one. + * + * So these exercise `postMessage` itself and assert the warn actually FIRES. + * That is the only form that distinguishes *wired* from *textually resembles + * being wired*, and it is also blind to nothing a rename can do. + * + * The predicate-level tests stay where they are — `sanitizeAgentContent`'s own + * suite pins WHEN each warn should fire. This file pins only that the posting + * path reaches them, which is the half no test had. + * + * Harness is the ~60-line `postMessage` mock set from + * `agentMessageService.phantom-directive.test.js`. + */ + +const AgentMessageService = require('../../../services/agentMessageService'); +const Message = require('../../../models/Message'); +const AgentIdentityService = require('../../../services/agentIdentityService'); +const socketConfig = require('../../../config/socket'); +const DMService = require('../../../services/dmService'); +const File = require('../../../models/File'); + +jest.mock('../../../models/Message'); +jest.mock('../../../models/Summary', () => ({ + findOne: jest.fn().mockResolvedValue(null), + create: jest.fn(), +})); +jest.mock('../../../services/agentIdentityService', () => ({ + getOrCreateAgentUser: jest.fn(), + ensureAgentInPod: jest.fn(), + buildAgentUsername: jest.fn((agentName, instanceId) => `${agentName}-${instanceId}`), +})); +jest.mock('../../../services/podAssetService', () => ({ + createChatSummaryAsset: jest.fn(), +})); +jest.mock('../../../config/socket', () => ({ getIO: jest.fn() })); +jest.mock('../../../services/dmService', () => ({ + resolveAgentOwner: jest.fn(), + getOrCreateAdminDMPod: jest.fn(), +})); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { + find: jest.fn(() => ({ + select: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue([]), + })), + }, +})); +jest.mock('../../../models/User', () => ({ + find: jest.fn(() => ({ + select: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue([]), + })), + findOne: jest.fn(() => ({ + select: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue(null), + })), +})); +jest.mock('../../../models/Pod', () => ({ + findById: jest.fn(() => ({ + select: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue({ type: 'chat' }), + })), +})); +jest.mock('../../../models/File', () => ({ + find: jest.fn(), + findOne: jest.fn(() => ({ + select: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue(null), + })), +})); + +const POD_ID = '6a0da39bae757028b39f87a6'; +let persistedDoc; +let warn; + +beforeEach(() => { + jest.clearAllMocks(); + persistedDoc = null; + warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + jest.spyOn(AgentMessageService, 'getRecentMessages').mockResolvedValue([]); + File.find.mockReturnValue({ + select: jest.fn().mockReturnThis(), + limit: jest.fn().mockReturnThis(), + lean: jest.fn().mockResolvedValue([]), + }); + AgentIdentityService.getOrCreateAgentUser.mockResolvedValue({ + _id: 'agent-user-1', + username: 'openclaw-nova', + profilePicture: 'default', + }); + AgentIdentityService.ensureAgentInPod.mockResolvedValue({ _id: 'pod-1' }); + socketConfig.getIO.mockReturnValue({ to: () => ({ emit: jest.fn() }) }); + Message.mockImplementation(function MockMessage(doc) { + persistedDoc = doc; + return { + ...doc, + _id: 'msg-1', + createdAt: new Date(), + save: jest.fn().mockResolvedValue(true), + populate: jest.fn().mockResolvedValue({ ...doc, _id: 'msg-1' }), + }; + }); + DMService.resolveAgentOwner.mockResolvedValue(null); + DMService.getOrCreateAdminDMPod.mockResolvedValue({ _id: 'dm-pod-1' }); +}); + +afterEach(() => { + warn.mockRestore(); + if (AgentMessageService.getRecentMessages.mockRestore) { + AgentMessageService.getRecentMessages.mockRestore(); + } +}); + +const warnsContaining = (needle) => warn.mock.calls + .map(([first]) => String(first)) + .filter((line) => line.includes(needle)); + +const post = (content) => AgentMessageService.postMessage({ + agentName: 'openclaw', + instanceId: 'nova', + podId: POD_ID, + content, +}); + +describe('postMessage delivers the sentinel-strip warn', () => { + it('fires, with identity, when a bare sentinel is edited out of a substantive reply', async () => { + await post('A reply of NO_REPLY means silence.'); + + const [line, ...rest] = warnsContaining('stripped bare sentinel'); + expect(line).toBeDefined(); + // Exactly one: the sanitizer runs once per post. A second line would mean + // the posting path sanitizes twice, which would double every count built + // on this warn. + expect(rest).toHaveLength(0); + // Identity is the whole opt-in — `observe` is what postMessage passes, and + // dropping it is the mutation the old source pin was written for. An + // anonymous line cannot be attributed to a seat and the metric dies. + expect(line).toContain('agent=openclaw'); + expect(line).toContain('instance=nova'); + expect(line).toContain(`pod=${POD_ID}`); + // And the message still posts, edited. The warn exists precisely because + // this is an edit rather than a suppression — if it were silently dropped + // there would be nothing to observe. + expect(persistedDoc.content).toBe('A reply of means silence.'); + }); + + it('stays silent through postMessage when the sentinel IS the whole reply', async () => { + // The control that stops this becoming "warns on every post". Total-match + // is suppression, not an edit: it must not inflate the count, or the rate + // measures ordinary heartbeat traffic. + await post('NO_REPLY'); + expect(warnsContaining('stripped bare sentinel')).toHaveLength(0); + expect(persistedDoc).toBeNull(); + }); + + it('stays silent through postMessage on a backticked sentinel and ordinary prose', async () => { + await post('Backtick it: `NO_REPLY` survives.'); + await post('An ordinary reply with no sentinel at all.'); + expect(warnsContaining('stripped bare sentinel')).toHaveLength(0); + }); +}); + +// The two sibling suppressions had no delivery pin of ANY kind — not even a +// source-text one. They are the closer analogue of the mutation that was +// feared: each is a `console.warn` immediately before `sanitizedContent = ''`, +// so deleting the warn and keeping the zeroing loses the whole record of a +// swallowed post while every predicate test stays green. +describe('postMessage delivers the two runtime-failure suppression warns', () => { + it('warns with identity and posts nothing for a runtime model failure', async () => { + await post('⚠️ Agent failed before reply: All models failed (4): openrouter/x: 401'); + + const [line] = warnsContaining('suppressed runtime model-failure'); + expect(line).toBeDefined(); + expect(line).toContain('agent=openclaw'); + expect(line).toContain('instance=nova'); + expect(line).toContain(`pod=${POD_ID}`); + expect(persistedDoc).toBeNull(); + }); + + it('warns with identity and posts nothing for a gateway tool-failure note', async () => { + await post('⚠️ 📝 Edit: in /workspace/nova/MEMORY.md (196 chars) failed'); + + const [line] = warnsContaining('suppressed runtime tool-failure note'); + expect(line).toBeDefined(); + expect(line).toContain('agent=openclaw'); + expect(line).toContain('instance=nova'); + expect(line).toContain(`pod=${POD_ID}`); + expect(persistedDoc).toBeNull(); + }); + + it('does not fire either suppression warn on an ordinary reply', async () => { + await post('The MEMORY.md edit failed twice, so I rewrote the file instead — done.'); + expect(warnsContaining('suppressed runtime model-failure')).toHaveLength(0); + expect(warnsContaining('suppressed runtime tool-failure note')).toHaveLength(0); + expect(persistedDoc).toBeTruthy(); + }); +});