diff --git a/backend/__tests__/unit/services/agentMessageService.selfIdentity.test.js b/backend/__tests__/unit/services/agentMessageService.selfIdentity.test.js new file mode 100644 index 00000000..074bc8ee --- /dev/null +++ b/backend/__tests__/unit/services/agentMessageService.selfIdentity.test.js @@ -0,0 +1,194 @@ +/** + * Regression tests for the "who posted?" family of bugs (#757). + * + * The BYO wrapper decides whether to echo its CLI output by asking the server + * for recent pod messages and looking for one it believes it wrote. Three + * separate defects made that answer wrong: + * + * 1. The wrapper could only test `isBot`, i.e. "did ANY bot post?", so in a + * multi-agent pod whichever agent finished second had its whole reply + * silently dropped. The fix is a server-computed `self` flag — the client + * cannot derive its own bot username safely (LEGACY_AGENT_MAP is + * server-only and instanceId is owner-scoped at install). + * 2. The Mongo fallback path populated `userId` WITHOUT `isBot`, so every + * message on that path reported `isBot: false` and self-post detection was + * dead outright — guaranteed double-posting whenever PG was unavailable. + * 3. The PG path fabricated `isBot` from a username substring test, so a + * human named "talbot" read as a bot and suppressed an agent's reply. + * + * Plus the sibling in hasRecentDuplicateUrls: it matched ANY author, so a + * human posting a link silenced the agent's next heartbeat post. + */ + +const AgentMessageService = require('../../../services/agentMessageService'); +const Message = require('../../../models/Message'); +const PGMessage = require('../../../models/pg/Message'); + +jest.mock('../../../models/Message'); +jest.mock('../../../models/pg/Message', () => ({ findByPodId: jest.fn() })); + +const POD = 'pod-1'; +const SELF = 'agent-user-1'; +const OTHER = 'agent-user-2'; + +// Mongo path: Message.find(...).sort(...).limit(...).populate(...).lean() +const mockMongoChain = (rows) => { + const populate = jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue(rows) }); + const limit = jest.fn().mockReturnValue({ populate }); + const sort = jest.fn().mockReturnValue({ limit }); + Message.find.mockReturnValue({ sort }); + return { populate }; +}; + +describe('getRecentMessages — PG path', () => { + const origPgHost = process.env.PG_HOST; + beforeEach(() => { + jest.clearAllMocks(); + process.env.PG_HOST = 'localhost'; + }); + afterAll(() => { + if (origPgHost === undefined) delete process.env.PG_HOST; + else process.env.PG_HOST = origPgHost; + }); + + test('stamps an explicit `self` boolean on every message when the caller identifies itself', async () => { + PGMessage.findByPodId.mockResolvedValue([ + { + id: 1, user_id: SELF, username: 'me-bot', is_bot: true, content: 'mine', + }, + { + id: 2, user_id: OTHER, username: 'other-bot', is_bot: true, content: 'theirs', + }, + ]); + + const out = await AgentMessageService.getRecentMessages(POD, 10, SELF); + + expect(out.map((m) => m.self)).toEqual([true, false]); + }); + + // Its absence is how a client detects a server too old to compute `self`. + test('omits `self` when no caller identity is given', async () => { + PGMessage.findByPodId.mockResolvedValue([ + { + id: 1, user_id: SELF, username: 'me-bot', is_bot: true, content: 'mine', + }, + ]); + + const out = await AgentMessageService.getRecentMessages(POD, 10); + + expect(out[0]).not.toHaveProperty('self'); + }); + + test('a human whose name merely ENDS IN "bot" is not reported as a bot', async () => { + // The old substring heuristic ('-bot' / endsWith 'bot' / 'openclaw-') + // misread these as bots, and isBot gates the wrapper's echo suppression. + PGMessage.findByPodId.mockResolvedValue([ + { + id: 1, user_id: 'u1', username: 'talbot', is_bot: false, content: 'hi', + }, + { + id: 2, user_id: 'u2', username: 'abbot', is_bot: false, content: 'hi', + }, + { + id: 3, user_id: 'u3', username: 'openclaw-aria', is_bot: false, content: 'hi', + }, + { + id: 4, user_id: 'u4', username: 'real-bot', is_bot: true, content: 'hi', + }, + ]); + + const out = await AgentMessageService.getRecentMessages(POD, 10); + + expect(out.map((m) => m.isBot)).toEqual([false, false, false, true]); + }); +}); + +describe('getRecentMessages — Mongo fallback path', () => { + const origPgHost = process.env.PG_HOST; + beforeEach(() => { + jest.clearAllMocks(); + delete process.env.PG_HOST; // force the Mongo branch + }); + afterAll(() => { + if (origPgHost === undefined) delete process.env.PG_HOST; + else process.env.PG_HOST = origPgHost; + }); + + // Without isBot in the projection, detection is dead and every turn double-posts. + test('selects isBot in the populate projection', async () => { + const { populate } = mockMongoChain([]); + + await AgentMessageService.getRecentMessages(POD, 10); + + expect(populate).toHaveBeenCalledWith('userId', expect.stringContaining('isBot')); + }); + + test('reports isBot truthfully for a bot author', async () => { + mockMongoChain([ + { _id: 'm1', content: 'x', userId: { _id: SELF, username: 'me-bot', isBot: true } }, + ]); + + const out = await AgentMessageService.getRecentMessages(POD, 10); + + expect(out[0].isBot).toBe(true); + }); + + test('stamps `self` on the Mongo path too', async () => { + mockMongoChain([ + { _id: 'm1', content: 'mine', userId: { _id: SELF, username: 'me-bot', isBot: true } }, + { _id: 'm2', content: 'theirs', userId: { _id: OTHER, username: 'other-bot', isBot: true } }, + ]); + + const out = await AgentMessageService.getRecentMessages(POD, 10, SELF); + + // .reverse() is applied on this path, so assert by author rather than order. + const mine = out.find((m) => String(m.userId._id) === SELF); + const theirs = out.find((m) => String(m.userId._id) === OTHER); + expect(mine.self).toBe(true); + expect(theirs.self).toBe(false); + }); +}); + +describe('hasRecentDuplicateUrls — author scoping', () => { + const origPgHost = process.env.PG_HOST; + beforeEach(() => { + jest.clearAllMocks(); + process.env.PG_HOST = 'localhost'; + }); + afterAll(() => { + if (origPgHost === undefined) delete process.env.PG_HOST; + else process.env.PG_HOST = origPgHost; + }); + + test('a link posted by SOMEONE ELSE no longer suppresses this author', async () => { + PGMessage.findByPodId.mockResolvedValue([ + { + id: 1, user_id: OTHER, username: 'human', is_bot: false, content: 'look: https://x.com/a', + }, + ]); + + const dup = await AgentMessageService.hasRecentDuplicateUrls({ + podId: POD, + content: 'https://x.com/a', + authorUserId: SELF, + }); + + expect(dup).toBe(false); + }); + + test("the author's OWN recent duplicate is still caught", async () => { + PGMessage.findByPodId.mockResolvedValue([ + { + id: 1, user_id: SELF, username: 'me-bot', is_bot: true, content: 'look: https://x.com/a', + }, + ]); + + const dup = await AgentMessageService.hasRecentDuplicateUrls({ + podId: POD, + content: 'https://x.com/a', + authorUserId: SELF, + }); + + expect(dup).toBe(true); + }); +}); diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index 8aaf171c..bceae27f 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -1103,7 +1103,8 @@ router.get( } const limit = Math.min(Math.max(parseInt(req.query.limit, 10) || 20, 1), 50); - const messages = await AgentMessageService.getRecentMessages(podId, limit); + // Same server-computed `self` flag as the runtime-token route (#757). + const messages = await AgentMessageService.getRecentMessages(podId, limit, user._id); return res.json({ messages }); } catch (error: any) { console.error('Error fetching bot messages:', error); @@ -1316,7 +1317,13 @@ router.get('/pods/:podId/messages', agentRuntimeAuth, async (req: any, res: any) } const limit = Math.min(Math.max(parseInt(req.query.limit, 10) || 20, 1), 50); - const messages = await AgentMessageService.getRecentMessages(podId, limit); + // Pass the caller's own user id so each message carries a server-computed + // `self` flag. The wrapper uses it to tell "I posted via my tool" from + // "some OTHER agent posted while I was thinking" — the latter used to + // silently swallow this agent's reply in any multi-agent pod (#757). + const messages = await AgentMessageService.getRecentMessages( + podId, limit, req.agentUser?._id, + ); return res.json({ messages }); } catch (error: any) { diff --git a/backend/services/agentMessageService.ts b/backend/services/agentMessageService.ts index 55cb9109..4bdb7378 100644 --- a/backend/services/agentMessageService.ts +++ b/backend/services/agentMessageService.ts @@ -146,6 +146,11 @@ interface MessageNormalized { userId: { _id: unknown; username: string; profilePicture?: string }; username: string; isBot?: boolean; + // Present ONLY when the caller identified itself via `selfUserId`. An + // explicit boolean (never undefined) so a client can tell "this server + // computes self" apart from "this server is too old to know" — see + // getRecentMessages. + self?: boolean; createdAt: Date | string; metadata?: MetadataDoc; profile_picture?: string; @@ -441,18 +446,35 @@ class AgentMessageService { }); } + /** + * "Has this author already shared these exact links recently?" — used to stop + * a heartbeat agent re-posting the same digest every tick. + * + * `authorUserId` scopes the lookback to that author's own messages. Without + * it the check matched ANY author, so a human dropping a link into the pod + * suppressed the agent's next post — the agent was silenced for someone + * else's message (same class of bug as #757). + */ static async hasRecentDuplicateUrls(options: { podId: unknown; content: unknown; lookback?: number; + authorUserId?: unknown; }): Promise { - const { podId, content, lookback = 10 } = options; + const { + podId, content, lookback = 10, authorUserId, + } = options; const urls = AgentMessageService.extractUrls(content); if (urls.length === 0) return false; try { const recent = await AgentMessageService.getRecentMessages(podId, lookback); if (!Array.isArray(recent) || recent.length === 0) return false; - const recentText = recent.map((m) => String(m?.content || '')).join('\n'); + const authorId = authorUserId ? String(authorUserId) : null; + const mine = authorId + ? recent.filter((m) => String(m?.userId?._id ?? '') === authorId) + : recent; + if (mine.length === 0) return false; + const recentText = mine.map((m) => String(m?.content || '')).join('\n'); const recentUrls = new Set(AgentMessageService.extractUrls(recentText)); return urls.every((url) => recentUrls.has(url)); } catch { @@ -1079,7 +1101,11 @@ class AgentMessageService { } if (isHeartbeatEvent) { - const isDuplicate = await AgentMessageService.hasRecentDuplicateUrls({ podId, content: sanitizedContent }); + const isDuplicate = await AgentMessageService.hasRecentDuplicateUrls({ + podId, + content: sanitizedContent, + authorUserId: agentUser?._id, + }); if (isDuplicate) { AgentMessageService.logMessageLifecycle('skipped', { agentName, @@ -1524,11 +1550,32 @@ class AgentMessageService { return cleaned; } - static async getRecentMessages(podId: unknown, limit = 20): Promise { + /** + * Recent pod messages, normalized across the PG and Mongo read paths. + * + * `selfUserId` — when the caller is a participant (an agent runtime asking + * "did *I* post during my turn?"), pass its user id and every returned + * message carries an explicit `self` boolean. Answering that server-side is + * deliberate: the client cannot safely derive its own bot username, because + * `resolveAgentType`'s LEGACY_AGENT_MAP is server-only and `instanceId` is + * owner-scoped and assigned at install. A client that guesses wrong silently + * double-posts. Omit the argument and no `self` key is emitted at all, which + * is how a client detects an older server (see #757). + */ + static async getRecentMessages( + podId: unknown, + limit = 20, + selfUserId?: unknown, + ): Promise { if (!podId) { throw new Error('podId is required'); } + const selfId = selfUserId ? String(selfUserId) : null; + const withSelf = (authorId: unknown): { self?: boolean } => ( + selfId ? { self: String(authorId ?? '') === selfId } : {} + ); + if (PGMessage && process.env.PG_HOST) { try { const messages: Array> = await (PGMessage as { @@ -1537,18 +1584,12 @@ class AgentMessageService { return messages.map((msg) => { const username = (msg.username as string) || 'Unknown'; - const isBot = - msg.is_bot === true - || msg.is_bot === 'true' - || (() => { - const lower = username.toLowerCase(); - return ( - lower.includes('-bot') - || lower.includes('_bot') - || lower.endsWith('bot') - || lower.startsWith('openclaw-') - ); - })(); + // Trust the mirrored `is_bot` column only. This used to fall back to + // a username substring test ('-bot', endsWith 'bot', 'openclaw-'), + // which misclassified any human named e.g. "talbot" or "abbot" as a + // bot — and `isBot` gates the wrapper's echo suppression, so a false + // positive silently ate that agent's reply. + const isBot = msg.is_bot === true || msg.is_bot === 'true'; return { _id: msg.id, id: msg.id, @@ -1561,6 +1602,7 @@ class AgentMessageService { }, username, isBot, + ...withSelf(msg.user_id), createdAt: msg.created_at as Date, }; }); @@ -1572,7 +1614,11 @@ class AgentMessageService { const messages: Array> = await Message.find({ podId }) .sort({ createdAt: -1 }) .limit(limit) - .populate('userId', 'username profilePicture') + // `isBot` MUST stay in this projection: it is read below, and omitting it + // made every message on this fallback path report isBot:false, which + // disabled the wrapper's self-post detection outright (guaranteed + // double-posting whenever PG was unavailable). + .populate('userId', 'username profilePicture isBot') .lean(); return messages.reverse().map((msg) => { @@ -1586,6 +1632,7 @@ class AgentMessageService { userId: userId || { username: 'Unknown' }, username, isBot: userId?.isBot === true, + ...withSelf(userId?._id), createdAt: msg.createdAt as Date, }; }) as MessageNormalized[]; diff --git a/cli/__tests__/run-loop.test.mjs b/cli/__tests__/run-loop.test.mjs index 5356b29b..8f1c35a0 100644 --- a/cli/__tests__/run-loop.test.mjs +++ b/cli/__tests__/run-loop.test.mjs @@ -234,6 +234,137 @@ describe('performRun', () => { ); }); + test('#757: ANOTHER agent posting during the spawn must NOT suppress this reply', async () => { + // The multi-agent-room case. Two wrapper agents are mentioned in one + // message and answer concurrently; the faster one lands its post inside + // the slower one's spawn window. The old detection asked "did any bot + // post?" and so threw away the slower agent's entire reply — silently, + // because the event was still acked. Sam hit this hand-sequencing his + // agents one at a time during a working session. + let messagesCall = 0; + const mockGet = jest.fn(async (route) => { + if (route === '/api/agents/runtime/memory') return { sections: {} }; + if (route.endsWith('/messages')) { + messagesCall += 1; + return messagesCall === 1 + ? { messages: [{ _id: 'm1', isBot: false, self: false }] } + : { + messages: [ + { _id: 'm1', isBot: false, self: false }, + // A DIFFERENT agent's post: bot-authored, but not ours. + { _id: 'm2', isBot: true, self: false, username: 'other-agent' }, + ], + }; + } + return { events: [makeEvent({ _id: 'evt-otheragent' })] }; + }); + const mockPost = jest.fn().mockResolvedValue({}); + createClient.mockReturnValue({ get: mockGet, post: mockPost }); + + const spawn = jest.fn(async () => ({ text: 'my real reply' })); + const adapter = { name: 'stub', detect: stubAdapter.detect, spawn }; + + const { stop } = performRun({ + instanceUrl: 'http://localhost:5000', + token: 'cm_agent_test', + adapter, + agentName: 'my-stub', + setTimeoutImpl: noopTimeout, + }); + await drainMicrotasks(); + stop(); + + expect(mockPost).toHaveBeenCalledWith( + '/api/agents/runtime/pods/pod-abc/messages', + { content: 'my real reply' }, + ); + }); + + test('#757: the agent OWN post (self: true) still suppresses the echo', async () => { + // The double-post guarantee the detection exists for must survive the fix. + let messagesCall = 0; + const mockGet = jest.fn(async (route) => { + if (route === '/api/agents/runtime/memory') return { sections: {} }; + if (route.endsWith('/messages')) { + messagesCall += 1; + return messagesCall === 1 + ? { messages: [{ _id: 'm1', isBot: false, self: false }] } + : { + messages: [ + { _id: 'm1', isBot: false, self: false }, + { _id: 'm2', isBot: true, self: true, username: 'my-stub' }, + ], + }; + } + return { events: [makeEvent({ _id: 'evt-ownpost' })] }; + }); + const mockPost = jest.fn().mockResolvedValue({}); + createClient.mockReturnValue({ get: mockGet, post: mockPost }); + + const spawn = jest.fn(async () => ({ text: 'narration of what I posted' })); + const adapter = { name: 'stub', detect: stubAdapter.detect, spawn }; + + const { stop } = performRun({ + instanceUrl: 'http://localhost:5000', + token: 'cm_agent_test', + adapter, + agentName: 'my-stub', + setTimeoutImpl: noopTimeout, + }); + await drainMicrotasks(); + stop(); + + expect(mockPost).not.toHaveBeenCalledWith( + '/api/agents/runtime/pods/pod-abc/messages', + { content: 'narration of what I posted' }, + ); + expect(mockPost).toHaveBeenCalledWith( + '/api/agents/runtime/events/evt-ownpost/ack', + { result: { outcome: 'posted' } }, + ); + }); + + test('#757: our own post still suppresses even when another agent posted too', async () => { + // Both happened in the window. Ours is what matters — suppress. + let messagesCall = 0; + const mockGet = jest.fn(async (route) => { + if (route === '/api/agents/runtime/memory') return { sections: {} }; + if (route.endsWith('/messages')) { + messagesCall += 1; + return messagesCall === 1 + ? { messages: [{ _id: 'm1', isBot: false, self: false }] } + : { + messages: [ + { _id: 'm1', isBot: false, self: false }, + { _id: 'm2', isBot: true, self: false, username: 'other-agent' }, + { _id: 'm3', isBot: true, self: true, username: 'my-stub' }, + ], + }; + } + return { events: [makeEvent({ _id: 'evt-bothposted' })] }; + }); + const mockPost = jest.fn().mockResolvedValue({}); + createClient.mockReturnValue({ get: mockGet, post: mockPost }); + + const spawn = jest.fn(async () => ({ text: 'narration' })); + const adapter = { name: 'stub', detect: stubAdapter.detect, spawn }; + + const { stop } = performRun({ + instanceUrl: 'http://localhost:5000', + token: 'cm_agent_test', + adapter, + agentName: 'my-stub', + setTimeoutImpl: noopTimeout, + }); + await drainMicrotasks(); + stop(); + + expect(mockPost).not.toHaveBeenCalledWith( + '/api/agents/runtime/pods/pod-abc/messages', + { content: 'narration' }, + ); + }); + test('ensures adapter cwd exists before spawning — avoids confusing spawn ENOENT on missing dir', async () => { const agentCwd = path.join(os.tmpdir(), 'commonly-agents', 'cwd-fresh-agent'); fs.rmSync(agentCwd, { recursive: true, force: true }); diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index 4361ec0a..518b348c 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -502,9 +502,10 @@ export const performRun = ({ // did, its final CLI text is a narration/log — echoing it would duplicate // the message and re-fire any @mention. This is the wrapper-side guarantee // that a deliberate multi-post ("on it…" then "…done") is never doubled, - // without relying on the agent to emit NO_REPLY. (A rare concurrent post by - // another member during the spawn window can suppress a genuine wrapper - // reply — an acceptable trade against a guaranteed double-post.) + // without relying on the agent to emit NO_REPLY. Against a server that + // stamps `self` this is exact; against an older one it degrades to the + // legacy any-bot test, where a concurrent post by another agent can + // suppress a genuine reply (#757). const snapshotMessages = async () => { try { const { messages = [] } = await client.get( @@ -551,18 +552,33 @@ export const performRun = ({ // reply via its output) the wrapper delivers the text as before. const replyText = (result.text || '').trim(); let agentPostedItself = false; + let suppressedBy = null; if (preSpawnIds) { const postSpawn = await snapshotMessages(); if (postSpawn) { + // The server stamps each message with `self` when it knows who is + // asking. Trust that over any local guess: the wrapper cannot derive + // its own bot username reliably (the server applies LEGACY_AGENT_MAP + // and an owner-scoped instanceId), and a wrong guess double-posts. + const serverKnowsSelf = postSpawn.some((m) => typeof m.self === 'boolean'); for (const m of postSpawn) { - // Only a NEW message from a bot user counts as "the agent posted - // itself". A new human-authored message must NOT suppress the echo: - // it is either a human typing mid-turn, or the agent misusing an - // operator CLI profile / human token (the 2026-07-22 as-operator - // attribution incident) — in both cases the wrapper still delivers - // the reply under the agent's own identity. - if (!preSpawnIds.has(String(m._id || m.id)) && m.isBot) { + if (preSpawnIds.has(String(m._id || m.id))) continue; + // A new human-authored message must NEVER suppress the echo: it is + // either a human typing mid-turn, or the agent misusing an operator + // CLI profile / human token (the 2026-07-22 as-operator attribution + // incident) — in both cases the wrapper still delivers the reply + // under the agent's own identity. + if (!m.isBot) continue; + // With `self`, only THIS agent's own post suppresses. Without it (an + // older server), fall back to the legacy any-bot test — which drops + // this agent's reply whenever another agent answers first (#757). + if (serverKnowsSelf ? m.self === true : true) { agentPostedItself = true; + suppressedBy = { + id: String(m._id || m.id), + author: m.username || 'unknown', + basis: serverKnowsSelf ? 'self' : 'legacy-any-bot', + }; break; } } @@ -571,7 +587,14 @@ export const performRun = ({ if (!replyText || replyText === 'NO_REPLY') { log(`[${event.type}] no wrapper-post (${replyText === 'NO_REPLY' ? 'NO_REPLY' : 'empty output'})`); } else if (agentPostedItself) { - log(`[${event.type}] agent posted via tool this turn — not echoing CLI output (avoids double-post)`); + // Name the message that caused the suppression. A silently dropped reply + // is invisible to everyone; #757 went unnoticed precisely because this + // line said only "posted via tool" with no way to tell whose post it saw. + log( + `[${event.type}] agent posted via tool this turn — not echoing CLI output ` + + `(avoids double-post; matched message ${suppressedBy.id} by ${suppressedBy.author} ` + + `via ${suppressedBy.basis})`, + ); } else { await client.post(`/api/agents/runtime/pods/${eventPodId}/messages`, { content: replyText,