From 1d9e0005df709d93c416d256a43cab5dafa1701c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:17:39 -0700 Subject: [PATCH 1/4] fix(webhook): inline reply posts through the CAP post path (TASK-015) deliverEventViaWebhook called agentMessageService.postAgentMessage, which does not exist. The TypeError was swallowed as a warning and the event still settled as outcome 'posted'. Inline replies now take the CAP route's path: the unseen-messages freshness hold, postMessage with the installation config, then the mention or agent-dm enqueue. A post that throws, or a reply held by the freshness gate, is recorded as error or skipped, never posted. Tests drive the real delivery through a static seam, with only the network and persistence edges mocked. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: sprint-review Agent-Model: claude/claude-haiku-5-5/default --- ...gentEventService.webhookInlinePost.test.js | 169 ++++++++++++++++++ backend/services/agentEventService.ts | 78 ++++++-- 2 files changed, 237 insertions(+), 10 deletions(-) create mode 100644 backend/__tests__/unit/services/agentEventService.webhookInlinePost.test.js diff --git a/backend/__tests__/unit/services/agentEventService.webhookInlinePost.test.js b/backend/__tests__/unit/services/agentEventService.webhookInlinePost.test.js new file mode 100644 index 0000000000..71ac10dfd4 --- /dev/null +++ b/backend/__tests__/unit/services/agentEventService.webhookInlinePost.test.js @@ -0,0 +1,169 @@ +/** + * TASK-015 — an inline webhook reply must actually post, and a failed post + * must not be recorded as posted. + * + * The old path called agentMessageService.postAgentMessage, which does not + * exist, so the TypeError was swallowed and the event still settled as + * `posted`. These tests drive the real deliverEventViaWebhook (through the + * static seam), with only the network and persistence edges mocked. + */ + +jest.mock('../../../models/AgentEvent', () => ({ + findByIdAndUpdate: jest.fn().mockResolvedValue({}), +})); + +jest.mock('../../../services/outboundUrlGuard', () => ({ + postToPublicAddress: jest.fn(), +})); + +jest.mock('../../../services/agentMessageService', () => ({ + postMessage: jest.fn(), +})); + +jest.mock('../../../services/agentMentionService', () => ({ + enqueueMentions: jest.fn().mockResolvedValue(undefined), + enqueueDmEvent: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('../../../services/agentSendFreshness', () => ({ + unseenMessages: jest.fn().mockResolvedValue(null), +})); + +jest.mock('../../../models/Pod', () => ({ + findById: jest.fn(), +})); + +jest.mock('../../../services/agentIdentityService', () => ({ + getOrCreateAgentUser: jest.fn(), +})); + +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn(), findOne: jest.fn() }, +})); + +jest.mock('../../../models/Integration', () => ({})); +jest.mock('../../../models/Gateway', () => ({ findById: jest.fn() })); +jest.mock('../../../services/agentDigestFoldService', () => ({ + foldExpiredDigests: jest.fn().mockResolvedValue(0), + readFoldedDigest: jest.fn().mockResolvedValue(null), + settleFoldedDigest: jest.fn().mockResolvedValue(undefined), +})); +jest.mock('../../../services/agentProvisionerService', () => ({ + clearAgentRuntimeSessions: jest.fn(), + restartAgentRuntime: jest.fn(), + resolveOpenClawAccountId: jest.fn(), +})); + +const AgentEvent = require('../../../models/AgentEvent'); +const { postToPublicAddress } = require('../../../services/outboundUrlGuard'); +const AgentMessageService = require('../../../services/agentMessageService'); +const AgentMentionService = require('../../../services/agentMentionService'); +const AgentSendFreshness = require('../../../services/agentSendFreshness'); +const PodModel = require('../../../models/Pod'); +const AgentIdentityService = require('../../../services/agentIdentityService'); +const AgentEventService = require('../../../services/agentEventService'); + +const installation = { + displayName: 'Echo', + config: { runtime: { webhookUrl: 'https://agent.example.com/hook' } }, +}; + +const event = { + _id: 'evt_inline_001', + type: 'chat.mention', + podId: 'pod_inline', + agentName: 'echo-agent', + instanceId: 'default', + createdAt: new Date().toISOString(), + payload: {}, +}; + +const agentUser = { _id: 'user_echo', username: 'echo-agent' }; + +const respondWith = (body) => postToPublicAddress.mockResolvedValue({ + ok: true, + status: 200, + text: JSON.stringify(body), +}); + +const recordedDelivery = () => { + const call = AgentEvent.findByIdAndUpdate.mock.calls.at(-1); + return call[1].delivery; +}; + +beforeEach(() => { + jest.clearAllMocks(); + AgentEventService.attachLegacyDigest = jest.fn().mockResolvedValue(undefined); + AgentEventService.settleLegacyDigest = jest.fn().mockResolvedValue(undefined); + AgentIdentityService.getOrCreateAgentUser.mockResolvedValue(agentUser); + AgentSendFreshness.unseenMessages.mockResolvedValue(null); + PodModel.findById.mockReturnValue({ + select: () => ({ lean: () => Promise.resolve({ type: 'team' }) }), + }); + AgentMessageService.postMessage.mockResolvedValue({ + success: true, + message: { _id: 'msg_1', content: 'hi' }, + }); +}); + +describe('webhook inline reply (TASK-015)', () => { + test('posted reply goes through postMessage and is recorded as posted', async () => { + respondWith({ outcome: 'posted', content: 'hi there' }); + + await AgentEventService.deliverEventViaWebhook(installation, event); + + expect(AgentMessageService.postMessage).toHaveBeenCalledWith(expect.objectContaining({ + agentName: 'echo-agent', + podId: 'pod_inline', + content: 'hi there', + installationConfig: installation.config, + })); + expect(recordedDelivery()).toEqual(expect.objectContaining({ outcome: 'posted' })); + }); + + test('a mention-enqueue runs for the posted reply, as on the CAP route', async () => { + respondWith({ outcome: 'posted', content: 'hi there' }); + + await AgentEventService.deliverEventViaWebhook(installation, event); + await new Promise((resolve) => { setImmediate(resolve); }); + + expect(AgentMentionService.enqueueMentions).toHaveBeenCalledWith(expect.objectContaining({ + podId: 'pod_inline', + userId: 'user_echo', + })); + }); + + test('a post that throws is recorded as error, never as posted', async () => { + AgentMessageService.postMessage.mockRejectedValue(new Error('db down')); + respondWith({ outcome: 'posted', content: 'hi there' }); + + await AgentEventService.deliverEventViaWebhook(installation, event); + + expect(recordedDelivery()).toEqual(expect.objectContaining({ + outcome: 'error', + reason: 'inline_post_failed', + })); + }); + + test('a reply held by the freshness gate is recorded as skipped, not posted', async () => { + AgentSendFreshness.unseenMessages.mockResolvedValue({ code: 'UNSEEN_MESSAGES' }); + respondWith({ outcome: 'posted', content: 'hi there' }); + + await AgentEventService.deliverEventViaWebhook(installation, event); + + expect(AgentMessageService.postMessage).not.toHaveBeenCalled(); + expect(recordedDelivery()).toEqual(expect.objectContaining({ + outcome: 'skipped', + reason: 'unseen_messages', + })); + }); + + test('a non-posted outcome never calls postMessage', async () => { + respondWith({ outcome: 'no_action', reason: 'nothing_to_add' }); + + await AgentEventService.deliverEventViaWebhook(installation, event); + + expect(AgentMessageService.postMessage).not.toHaveBeenCalled(); + expect(recordedDelivery()).toEqual(expect.objectContaining({ outcome: 'no_action' })); + }); +}); diff --git a/backend/services/agentEventService.ts b/backend/services/agentEventService.ts index e048016bc7..37317b94fa 100644 --- a/backend/services/agentEventService.ts +++ b/backend/services/agentEventService.ts @@ -194,6 +194,7 @@ interface InstallationDoc { _id?: unknown; agentName?: string; instanceId?: string; + displayName?: string; podId?: unknown; status?: string; scopes?: string[]; @@ -315,6 +316,55 @@ const normalizeConfig = (config: unknown): Record => { return config as Record; }; +// Inline webhook replies take the same service path as the CAP post route +// (agentsRuntime.ts): the unseen-messages freshness hold, postMessage with the +// installation config, then the mention or agent-dm enqueue. Resolves to a +// recorded outcome only when the reply was held or skipped; throws when the post fails. +const postInlineReply = async ( + installation: InstallationDoc, event: EventDoc, content: string, +): Promise<{ outcome: string; reason: string } | null> => { + // eslint-disable-next-line global-require + const AgentMessageService = require('./agentMessageService'); + // eslint-disable-next-line global-require + const AgentMentionService = require('./agentMentionService'); + // eslint-disable-next-line global-require + const AgentSendFreshness = require('./agentSendFreshness'); + // eslint-disable-next-line global-require + const PodModel = require('../models/Pod'); + + const agentUser = await AgentIdentityService.getOrCreateAgentUser(event.agentName, { + instanceId: event.instanceId || 'default', + displayName: installation.displayName, + }); + const hold = await AgentSendFreshness.unseenMessages(agentUser._id, event.podId); + if (hold) return { outcome: 'skipped', reason: 'unseen_messages' }; + + const result = await AgentMessageService.postMessage({ + agentName: event.agentName, + instanceId: event.instanceId || 'default', + displayName: installation.displayName, + podId: event.podId, + content, + replyToMessageId: null, + threadRootId: null, + installationConfig: installation.config || null, + }); + if (result?.skipped) return { outcome: 'skipped', reason: String(result.reason || 'skipped') }; + if (!result?.success || !result.message) throw new Error('post returned no message'); + + PodModel.findById(event.podId).select('type').lean() + .then((podDoc: { type?: string } | null) => { + const args = { + podId: event.podId, message: result.message, userId: agentUser._id, username: agentUser.username, + }; + return podDoc?.type === 'agent-dm' + ? AgentMentionService.enqueueDmEvent(args) + : AgentMentionService.enqueueMentions(args); + }) + .catch((err: Error) => console.warn('[webhook] inline reply enqueue failed:', err.message)); + return null; +}; + const deliverEventViaWebhook = async (installation: InstallationDoc, event: EventDoc): Promise => { const runtimeConfig = (normalizeConfig(installation.config)?.runtime || {}) as Record; const { webhookUrl, webhookSecret } = runtimeConfig as { webhookUrl?: string; webhookSecret?: string }; @@ -366,18 +416,21 @@ const deliverEventViaWebhook = async (installation: InstallationDoc, event: Even outcome?: string; content?: string; reason?: string; }; + // The endpoint's claim is only what it asked for. If the inline reply does + // not land, record that instead of 'posted'. + let recordedOutcome = outcome; + let recordedReason = reason; if (outcome === 'posted' && content) { try { - // eslint-disable-next-line global-require - const agentMessageService = require('./agentMessageService'); - await agentMessageService.postAgentMessage({ - agentName: event.agentName, - instanceId: event.instanceId, - podId: event.podId, - content: String(content), - }); + const held = await postInlineReply(installation, event, String(content)); + if (held) { + recordedOutcome = held.outcome; + recordedReason = held.reason; + } } catch (postErr) { console.warn(`[webhook] Failed to post message for ${event.agentName}:`, (postErr as Error).message); + recordedOutcome = 'error'; + recordedReason = 'inline_post_failed'; } } @@ -394,9 +447,9 @@ const deliverEventViaWebhook = async (installation: InstallationDoc, event: Even await AgentEvent.findByIdAndUpdate(event._id, { status: 'acked', deliveredAt: new Date(), - delivery: AgentEventService.normalizeDeliveryMeta({ outcome, reason }), + delivery: AgentEventService.normalizeDeliveryMeta({ outcome: recordedOutcome, reason: recordedReason }), }); - if (digestConsumedBy(outcome, reason)) { + if (digestConsumedBy(recordedOutcome, recordedReason)) { await AgentEventService.settleLegacyDigest(event).catch((err: Error) => { console.warn('[webhook] digest settle failed:', err.message); }); @@ -809,6 +862,11 @@ class AgentEventService { }; } + // Test seam: unit tests drive the real webhook delivery, not a copy of it. + static deliverEventViaWebhook(installation: InstallationDoc, event: EventDoc): Promise { + return deliverEventViaWebhook(installation, event); + } + static normalizeDeliveryMeta(input: unknown = {}): DeliveryMeta | null { if (!input || typeof input !== 'object') return null; const allowedOutcomes = new Set(['acknowledged', 'posted', 'no_action', 'skipped', 'error']); From fc7ce843ab745c08767622f9161e381525a7c8d7 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:18:07 -0700 Subject: [PATCH 2/4] docs(review): rule against tests that re-implement the function under test (TASK-015) Co-Authored-By: Claude Haiku 5.5 Co-authored-by: sprint-review Agent-Model: claude/claude-haiku-5-5/default --- docs/development/review-checklist.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 03bddfac0e..d5afa0a03a 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -250,3 +250,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## Who installs the CLI 58. **A guard or default meant for our own fleet ships in the same public CLI that outside users install with `@latest`, so its review must run the public Bring-your-own command exactly as the page prints it, with no flags and no prior configuration.** Checking the fleet's own token files or runners shows what the change does to *our* seats. It says nothing about a first-time user, whose attach carries none of the fields our seats declare. A fleet-only behaviour belongs behind something only the fleet sets (an environment variable in the fleet's runner scripts, a fleet marker), and the PR carries a test that the unflagged public path behaves as before. *(Earned: #2662, 2026-10-25. A guard that refused any adapter turn without `environment.model` was specced, implemented, reviewed and gated against our seats, where every running seat declared a model and the 14 that didn't were idle. It shipped as cli 0.1.163. The Bring-your-own page attaches with no model, so every new user's Claude Code went green and then never answered. A GTM walk of the page found it 40 minutes later. #2663 reverted it as 0.1.164, since a dist-tag can't be moved by hand under OIDC-only publishing, and #2665 relanded it behind `COMMONLY_REQUIRE_SEAT_MODEL=1`, with a test that a model-less public attach still answers.)* + +## Tests that exercise a copy + +59. **A test that re-implements the function under test passes whatever the function does.** A "minimal re-implementation" of a delivery or write path, kept beside the real one for isolation, is a second function that the suite certifies. The real function can fail in any way and the suite stays green, because nothing it calls is the real thing. Before approving a suite, check that the function named in each test is the one the service exports. Where the real function is not reachable, expose a seam (a static method, not a module export) and drive it with only the network and persistence edges mocked. *(Earned: TASK-015, 2026-10-26. `webhook-delivery.test.js` carried a `deliver()` copy and asserted its response handling. The production path called `agentMessageService.postAgentMessage`, which does not exist, and the TypeError was swallowed, so every webhook reply marked `posted` and never reached the pod. The copy had no such call, so the suite was green on the bug.)* From d1e046905a42094308696ca5923ec309efd1736b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 02:28:07 -0700 Subject: [PATCH 3/4] fix(webhook): the delivered trigger is seen, so an inline reply is not held by it (TASK-015) An inline webhook reply always counted the message that woke the agent as unseen, so the freshness hold recorded every reply as a skip. The reply answers that message, so unseenMessages now takes the trigger's id and holds only on messages after it (PG id and Mongo _id). A non-ObjectId or non-numeric trigger is ignored, leaving the old bound. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: sprint-review Agent-Model: claude/claude-haiku-5-5/default --- .../unit/services/agentSendFreshness.test.js | 38 +++++++++++++++++++ backend/services/agentEventService.ts | 6 ++- backend/services/agentSendFreshness.ts | 19 ++++++++-- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/unit/services/agentSendFreshness.test.js b/backend/__tests__/unit/services/agentSendFreshness.test.js index 60e9c60e68..84da309e74 100644 --- a/backend/__tests__/unit/services/agentSendFreshness.test.js +++ b/backend/__tests__/unit/services/agentSendFreshness.test.js @@ -139,3 +139,41 @@ describe('agent send-time freshness', () => { }); }); + +describe('a delivered trigger is seen through its own message (TASK-015 inline reply)', () => { + beforeEach(() => { + jest.clearAllMocks(); + delete process.env.PG_HOST; + Cursor.getReadWatermark.mockResolvedValue({ readAt: new Date('2026-10-24T00:00:00Z') }); + Message.find.mockReturnValue({ sort: () => ({ limit: () => ({ populate: () => ({ lean: async () => [] }) }) }) }); + }); + + it('does not hold on the trigger itself: the query excludes it by id', async () => { + Message.countDocuments.mockResolvedValue(0); + const triggerId = '6adfeccc58eabb3645d76520'; + + const hold = await Freshness.unseenMessages('agent-id', 'pod-id', { seenThroughMessageId: triggerId }); + expect(hold).toBeNull(); + expect(Message.countDocuments.mock.calls[0][0]._id).toEqual({ $gt: triggerId }); + }); + + it('still holds on a message that landed after the trigger', async () => { + Message.countDocuments.mockResolvedValue(1); + const hold = await Freshness.unseenMessages('agent-id', 'pod-id', { + seenThroughMessageId: '6adfeccc58eabb3645d76520', + }); + expect(hold.unseenCount).toBe(1); + }); + + it('without a trigger the query has no id bound, so the old behaviour is unchanged', async () => { + Message.countDocuments.mockResolvedValue(0); + await Freshness.unseenMessages('agent-id', 'pod-id'); + expect(Message.countDocuments.mock.calls[0][0]).not.toHaveProperty('_id'); + }); + + it('a non-ObjectId trigger is ignored rather than becoming an unbounded filter', async () => { + Message.countDocuments.mockResolvedValue(0); + await Freshness.unseenMessages('agent-id', 'pod-id', { seenThroughMessageId: { $ne: 1 } }); + expect(Message.countDocuments.mock.calls[0][0]).not.toHaveProperty('_id'); + }); +}); diff --git a/backend/services/agentEventService.ts b/backend/services/agentEventService.ts index 37317b94fa..f42539e78d 100644 --- a/backend/services/agentEventService.ts +++ b/backend/services/agentEventService.ts @@ -336,7 +336,11 @@ const postInlineReply = async ( instanceId: event.instanceId || 'default', displayName: installation.displayName, }); - const hold = await AgentSendFreshness.unseenMessages(agentUser._id, event.podId); + // The event's own message is the one being answered, so only messages that + // landed after it hold the reply (see unseenMessages). + const hold = await AgentSendFreshness.unseenMessages(agentUser._id, event.podId, { + seenThroughMessageId: event.payload?.messageId, + }); if (hold) return { outcome: 'skipped', reason: 'unseen_messages' }; const result = await AgentMessageService.postMessage({ diff --git a/backend/services/agentSendFreshness.ts b/backend/services/agentSendFreshness.ts index c50329a2f9..7abfefb8f6 100644 --- a/backend/services/agentSendFreshness.ts +++ b/backend/services/agentSendFreshness.ts @@ -104,7 +104,12 @@ async function recordCatchUp( return { skippedCount }; } -async function unseenMessages(agentUserId: unknown, podId: unknown): Promise { +// A delivered event names the message that woke the agent. That message is the +// one the reply answers, so it is treated as seen: only messages after it hold. +// Without this, an inline webhook reply always counts its own trigger as unseen. +async function unseenMessages( + agentUserId: unknown, podId: unknown, { seenThroughMessageId = null }: { seenThroughMessageId?: unknown } = {}, +): Promise { const cursor = await Cursor.getReadWatermark(agentUserId, podId); const after = cursor?.readAt || new Date(0); const afterRevision = cursor?.readRevision ?? -1; @@ -113,6 +118,7 @@ async function unseenMessages(agentUserId: unknown, podId: unknown): Promise $2 AND (m.user_id IS DISTINCT FROM $3 OR m.author_kind = 'kernel') AND NOT (m.message_type = 'system' AND COALESCE(m.payload->>'kind', '') = 'system-action') + AND ($4::integer IS NULL OR m.id > $4) ORDER BY m.freshness_revision DESC, m.id DESC LIMIT 3 - `, [String(podId), afterRevision, ownId]); + `, [String(podId), afterRevision, ownId, seenPgId]); unseenCount = Number(result.rows[0]?.unseen_count || 0); messages = result.rows.map((row: Record) => ({ id: String(row.id), @@ -130,7 +137,8 @@ async function unseenMessages(agentUserId: unknown, podId: unknown): Promise { +async function unseenFromMongo( + podId: unknown, ownId: string, after: Date, seenMongoId: string | null, +): Promise<{ unseenCount: number; messages: Preview[] }> { const query = { podId, + ...(seenMongoId ? { _id: { $gt: seenMongoId } } : {}), $nor: [{ messageType: 'system', 'payload.kind': 'system-action' }], $and: [ { $or: [{ userId: { $ne: ownId } }, { authorKind: 'kernel' }] }, From a7a06a11476ed47594bc887df5886ba9ac5b78f4 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:27:20 -0700 Subject: [PATCH 4/4] test(freshness): pin the PostgreSQL trigger-id bound on unseenMessages (TASK-015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sable's HOLD on #2797: the $4 seen-through clause was the only thing stopping the trigger counting as unseen under PostgreSQL. Assert the bound id is passed, a non-numeric id becomes null, and no id leaves the clause unbounded. Written by sprint-review, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf Co-Authored-By: Claude Haiku 5.5 Co-authored-by: sprint-review Agent-Model: claude/claude-haiku-5-5/default --- .../unit/services/agentSendFreshness.test.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/backend/__tests__/unit/services/agentSendFreshness.test.js b/backend/__tests__/unit/services/agentSendFreshness.test.js index 84da309e74..bbfe7a4b24 100644 --- a/backend/__tests__/unit/services/agentSendFreshness.test.js +++ b/backend/__tests__/unit/services/agentSendFreshness.test.js @@ -97,6 +97,18 @@ describe('agent send-time freshness', () => { expect(Message.countDocuments).not.toHaveBeenCalled(); }); + it('bounds the PostgreSQL unseen query by the trigger id, and only a numeric id', async () => { + process.env.PG_HOST = 'configured'; + pool.query.mockResolvedValue({ rows: [] }); + await Freshness.unseenMessages('agent-id', 'pod-id', { seenThroughMessageId: '22' }); + expect(pool.query.mock.calls[0][1]).toEqual(['pod-id', -1, 'agent-id', 22]); + expect(pool.query.mock.calls[0][0]).toContain('($4::integer IS NULL OR m.id > $4)'); + await Freshness.unseenMessages('agent-id', 'pod-id', { seenThroughMessageId: 'abc' }); + expect(pool.query.mock.calls[1][1][3]).toBeNull(); + await Freshness.unseenMessages('agent-id', 'pod-id'); + expect(pool.query.mock.calls[2][1][3]).toBeNull(); + }); + it('captures the PostgreSQL revision before the read and advances it with the cursor', async () => { process.env.PG_HOST = 'configured'; pool.query.mockResolvedValueOnce({ rows: [{ revision: '7' }] }).mockResolvedValueOnce({ rows: [] });