From d331b16dd905697e4e056640d325afbdf0e47b59 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 04:22:38 -0700 Subject: [PATCH 1/2] fix(agents): a suppressed runtime failure must not conclude an agent-dm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review found that agentMessageService empties sanitizedContent for a runtime model-failure (:949) and a tool-failure note (:957), and the empty path returns the same `silent_or_empty` an intentional NO_REPLY returns. The label was the small half. The large half: that 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, attributing a takeaway the agent never reached. A failure laundered into a positive semantic event, with a console.warn as its only honest trace — a surface no agent can read. Two arrivals at `''` that mean opposite things: an intentional NO_REPLY is the agent deciding the exchange is over; a suppressed failure is the agent never having produced a reply. Now tracked and discriminated. A failure concludes nothing and names itself: runtime_model_failure_suppressed / runtime_tool_failure_suppressed. Still success+skipped, because the caller did nothing wrong and a 500 would turn a degraded model chain into a broken endpoint. Both existing silent_or_empty assertions (clawdbot-e2e :732, :748) are genuine silences and keep the old value. CONTROL in the suite: a bare NO_REPLY and whitespace-only content must STILL conclude, or the fix trades one collapse for another. Mutation- checked: reverting the discrimination reddens 4 of 7, and the two CONTROL cases stay green. 14 tests green with the chatNoise sibling suite. Co-Authored-By: Claude Opus 5 --- ....suppressedFailureIsNotAConclusion.test.js | 83 +++++++++++++++++++ backend/services/agentMessageService.ts | 26 ++++++ 2 files changed, 109 insertions(+) create mode 100644 backend/__tests__/unit/services/agentMessageService.suppressedFailureIsNotAConclusion.test.js 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..5711291af 100644 --- a/backend/services/agentMessageService.ts +++ b/backend/services/agentMessageService.ts @@ -946,9 +946,14 @@ 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 `''`. + 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 +962,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 @@ -1122,6 +1128,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 From 8fd4b3d0a6f039250f9935d1f455282e781ad0d9 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 04:24:24 -0700 Subject: [PATCH 2/2] docs(agents): connect the two failure-visibility guards in this function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review: the exemplar is 160 lines below the defect. The phantom- upload guard appends a VISIBLE system note to the message AND warns — both audiences, deliberately. The runtime-failure suppression warns only. Same file, same function, both answers, nothing connecting them. The asymmetry is defensible and must not be "fixed" by symmetry: the phantom guard has a message to annotate, the suppression would have to manufacture a post, which is the ~30-min-per-pod flood it exists to stop. Recorded in both directions so the next author sees that the question has already been answered once here, and on what grounds. What was never defensible is two guards deciding who hears a failure, independently, in one function, with no pointer between them. 93 tests green across the agentMessageService suites. Co-Authored-By: Claude Opus 5 --- backend/services/agentMessageService.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/backend/services/agentMessageService.ts b/backend/services/agentMessageService.ts index 5711291af..5aa55a629 100644 --- a/backend/services/agentMessageService.ts +++ b/backend/services/agentMessageService.ts @@ -948,6 +948,17 @@ class AgentMessageService { // 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)) { @@ -1116,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)}`); } }