diff --git a/backend/__tests__/unit/services/agentMessageService.suppressedFailureIsNotAConclusion.test.js b/backend/__tests__/unit/services/agentMessageService.suppressedFailureIsNotAConclusion.test.js new file mode 100644 index 000000000..4dec148ef --- /dev/null +++ b/backend/__tests__/unit/services/agentMessageService.suppressedFailureIsNotAConclusion.test.js @@ -0,0 +1,83 @@ +/** + * A suppressed runtime failure must not conclude an agent-dm. + * + * @sprint-review, 2026-08-25: `agentMessageService` empties `sanitizedContent` + * for a runtime model-failure (:949) and a tool-failure note (:957), and the + * empty path then returns the SAME `silent_or_empty` an intentional NO_REPLY + * returns. The label was the small half. + * + * The large half is that the empty path also fires ADR-012 §4's + * `recordAgentDmConclusion`, which writes a `system_exchanges` entry for BOTH + * peers whose takeaway is the sender's PRECEDING message. So a model chain + * being exhausted wrote "this conversation concluded" into the record, with a + * takeaway the agent never reached — a failure laundered into a positive + * semantic event, its only honest trace a `console.warn` no agent can read. + * + * These pin the discrimination in both directions. The NO_REPLY case is the + * control: it must still conclude, or the fix has traded one collapse for + * another. + */ +const mockRecordAgentDmConclusion = jest.fn().mockResolvedValue(undefined); +jest.mock('../../../services/systemExchangeTriggers', () => ({ + recordAgentDmConclusion: (...args) => mockRecordAgentDmConclusion(...args), +})); + +const AgentMessageService = require('../../../services/agentMessageService'); + +const POD = '69ef02b036b742e2e2c0c4af'; +const post = (content) => AgentMessageService.postMessage({ + agentName: 'openclaw', instanceId: 'nova', podId: POD, content, +}); + +beforeEach(() => { + mockRecordAgentDmConclusion.mockClear(); + jest.spyOn(console, 'warn').mockImplementation(() => {}); +}); +afterEach(() => jest.restoreAllMocks()); + +describe('a failure concludes nothing', () => { + it.each([ + ['model chain exhausted', '⚠️ Agent failed before reply: All models failed (4): openrouter/x: 401'], + ['failover summary', 'All models failed (3): openrouter/a: 429'], + ['tool-status note', '⚠️ 📝 Edit: in /workspace/MEMORY.md failed'], + ])('%s does not fire the agent-dm conclusion trigger', async (_label, content) => { + await post(content); + expect(mockRecordAgentDmConclusion).not.toHaveBeenCalled(); + }); + + it('and says which failure it was, not "silent"', async () => { + const model = await post('All models failed (3): openrouter/a: 429'); + const tool = await post('⚠️ 📝 Edit: in /workspace/MEMORY.md failed'); + expect(model.reason).toBe('runtime_model_failure_suppressed'); + expect(tool.reason).toBe('runtime_tool_failure_suppressed'); + // Still skipped, still not an error — the caller did nothing wrong and a + // 500 here would turn a degraded model chain into a broken endpoint. + expect(model).toMatchObject({ success: true, skipped: true }); + expect(tool).toMatchObject({ success: true, skipped: true }); + }); + + it('logs the suppressed content for the operator, as before', async () => { + await post('All models failed (3): openrouter/a: 429'); + const warned = console.warn.mock.calls.map((c) => String(c[0])).join('\n'); + expect(warned).toContain('suppressed runtime model-failure'); + expect(warned).toContain('All models failed'); + }); +}); + +describe('CONTROL: an intentional silence still concludes', () => { + // Without this the suite is equally consistent with a fix that simply + // stopped calling the trigger, which would break ADR-012 §4 outright. + it('a bare NO_REPLY fires the trigger and reports silent_or_empty', async () => { + const res = await post('NO_REPLY'); + expect(mockRecordAgentDmConclusion).toHaveBeenCalledWith({ + podId: POD, senderAgentName: 'openclaw', senderInstanceId: 'nova', + }); + expect(res.reason).toBe('silent_or_empty'); + }); + + it('and so does genuinely empty content', async () => { + const res = await post(' '); + expect(mockRecordAgentDmConclusion).toHaveBeenCalled(); + expect(res.reason).toBe('silent_or_empty'); + }); +}); diff --git a/backend/services/agentMessageService.ts b/backend/services/agentMessageService.ts index 4d3a46776..5aa55a629 100644 --- a/backend/services/agentMessageService.ts +++ b/backend/services/agentMessageService.ts @@ -946,9 +946,25 @@ class AgentMessageService { // spam every pod the agent heartbeats in (~every 30 min). Treat them like a // silent reply (empty → skipped below) and log instead, so a degraded // community agent fails quietly rather than flooding chat. + // WHY the content emptied, kept because the empty path below cannot + // otherwise tell a failure from a decision. Both arrive as `''`. + // + // Read alongside the phantom-upload guard ~160 lines down, which faces the + // same question and answers it the other way: it appends a VISIBLE system + // note to the message and warns, so the pod and the operator both learn. + // This pair warns only. The asymmetry is defensible and is NOT a bug to + // "fix" by symmetry — that guard has a message to annotate, and this one + // would have to manufacture a post, which is the ~30-minute-per-pod flood + // the suppression exists to stop. What was never defensible was the two + // guards being written independently in one function with nothing pointing + // at each other, so the second author could not see the first had already + // decided who hears a failure. (@sprint-review, 2026-08-25.) + let suppressedFailure: 'model_failure' | 'tool_failure' | null = null; + if (sanitizedContent && AgentMessageService.isRuntimeModelFailure(sanitizedContent)) { console.warn(`[agent-msg] suppressed runtime model-failure from agent=${agentName} instance=${instanceId} pod=${podId}: ${sanitizedContent.slice(0, 120)}`); sanitizedContent = ''; + suppressedFailure = 'model_failure'; } // Same treatment for gateway tool-status failure notes ("⚠️ 📝 Edit: ... @@ -957,6 +973,7 @@ class AgentMessageService { if (sanitizedContent && AgentMessageService.isRuntimeToolFailureNote(sanitizedContent)) { console.warn(`[agent-msg] suppressed runtime tool-failure note from agent=${agentName} instance=${instanceId} pod=${podId}: ${sanitizedContent.slice(0, 120)}`); sanitizedContent = ''; + suppressedFailure = 'tool_failure'; } // Task #68: detect false-attachment claims. Agents sometimes post @@ -1110,6 +1127,10 @@ class AgentMessageService { if (phantoms.length > 0) { const phantomList = phantoms.map((n) => `\`${n}\``).join(', '); sanitizedContent += `\n\n⚠️ _(system note: this message references ${phantoms.length === 1 ? 'an upload directive' : 'upload directives'} for ${phantomList} but no matching attachment was found in this pod. The agent may have typed the directive without actually calling \`commonly_attach_file\`. Check the agent's workspace.)_`; + // Both audiences on purpose: the note tells the pod, the warn + // tells the operator. The runtime-failure suppression at the top + // of this function deliberately does only the second — see the + // note there for why the two differ and why that is not drift. console.warn(`[agent-msg] phantom-upload-directive from agent=${agentName} instance=${instanceId} pod=${podId} — names=${JSON.stringify(phantoms)}`); } } @@ -1122,6 +1143,26 @@ class AgentMessageService { } if (!sanitizedContent) { + // Two ways to arrive here and they mean opposite things. An intentional + // NO_REPLY is the agent DECIDING the exchange is over. A suppressed + // runtime failure is the agent never having produced a reply at all — + // the model chain was exhausted, or a tool note was relayed as prose. + // + // Collapsing them cost more than a wrong label. ADR-012 §4 fires here: + // in an agent-dm both peers get a `system_exchanges` conclusion entry + // whose takeaway is the SENDER's PRECEDING message. So a model-chain + // exhaustion used to write "this conversation concluded" into the + // record, for both peers, attributing a takeaway the agent never + // reached — a failure laundered into a positive semantic event. The + // caller meanwhile got the same `silent_or_empty` an intentional + // NO_REPLY returns, and the only trace of the failure was a + // `console.warn` no agent can read. (@sprint-review, 2026-08-25.) + // + // So: a failure concludes nothing, and says which failure it was. + if (suppressedFailure) { + return { success: true, skipped: true, reason: `runtime_${suppressedFailure}_suppressed` }; + } + // ADR-012 §4: agent-dm-conclusion trigger. The reply is silent // (NO_REPLY swallowed by sanitizeAgentContent). If the pod is an // agent-dm, both peers get a system_exchanges entry whose takeaway is