diff --git a/backend/__tests__/unit/routes/agentsRuntime.ackDeliveryNonce.test.js b/backend/__tests__/unit/routes/agentsRuntime.ackDeliveryNonce.test.js new file mode 100644 index 0000000000..d9f356b795 --- /dev/null +++ b/backend/__tests__/unit/routes/agentsRuntime.ackDeliveryNonce.test.js @@ -0,0 +1,158 @@ +/** + * ADR-026 D6 at the route boundary. + * + * The service can refuse an ack in two ways that both surface as `null`: + * the event is gone (idempotent success) or the caller's delivery was + * superseded / its nonce was missing under Phase B. A route that answers + * `200 { success: true }` to all three tells a driver it acked when it did + * not — the event then rolls into the requeue with nobody informed, which is + * the silent-failure shape D6 exists to remove. + */ +const express = require('express'); +const request = require('supertest'); + +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); +jest.mock('../../../middleware/auth', () => (req, res, next) => next()); +jest.mock('../../../middleware/apiTokenScopes', () => ({ + requireApiTokenScopes: () => (req, res, next) => next(), +})); +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, _res, next) => { + req.agentUser = { botMetadata: { agentName: 'pixel', instanceId: 'default' } }; + next(); +}); + +const mockAcknowledge = jest.fn(); +const mockIsSuperseded = jest.fn(); +const mockIsRequired = jest.fn(); +const mockUserFindById = jest.fn(); +jest.mock('../../../services/agentEventService', () => ({ + acknowledge: (...a) => mockAcknowledge(...a), + isSupersededDelivery: (...a) => mockIsSuperseded(...a), + isDeliveryNonceRequired: (...a) => mockIsRequired(...a), +})); + +jest.mock('../../../services/agentIdentityService', () => ({ + buildAgentUsername: (agentName, instanceId) => (instanceId === 'default' ? agentName : `${agentName}-${instanceId}`), +})); +jest.mock('../../../services/agentMessageService', () => ({})); +jest.mock('../../../services/agentThreadService', () => ({})); +jest.mock('../../../services/podContextService', () => ({})); +jest.mock('../../../services/globalModelConfigService', () => ({})); +jest.mock('../../../services/socialPolicyService', () => ({})); +jest.mock('../../../services/messageClaimService', () => ({})); +jest.mock('../../../services/agentTypingService', () => ({})); +jest.mock('../../../services/dmService', () => ({})); +jest.mock('../../../integrations', () => ({ get: jest.fn() })); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/User', () => ({ findById: (...a) => mockUserFindById(...a) })); +jest.mock('../../../models/Post', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../models/Integration', () => ({ find: jest.fn(), findOne: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { findOne: jest.fn(), find: jest.fn() }, +})); + +const app = express(); +app.use(express.json()); +app.use('/api/agents/runtime', require('../../../routes/agentsRuntime')); + +const ACK = '/api/agents/runtime/events/evt-1/ack'; +const BOT_ACK = '/api/agents/runtime/bot/events/evt-1/ack'; + +describe('POST /events/:id/ack — delivery nonce', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockIsRequired.mockReturnValue(false); + mockIsSuperseded.mockResolvedValue(false); + mockAcknowledge.mockResolvedValue({ _id: 'evt-1', status: 'acked' }); + }); + + test('passes the deliveryId and declared consumer through', async () => { + const res = await request(app) + .post(ACK) + .set('x-commonly-client', 'cli') + .send({ deliveryId: 'abc123', result: { outcome: 'posted' } }); + + expect(res.status).toBe(200); + expect(mockAcknowledge).toHaveBeenCalledWith('evt-1', 'pixel', 'default', { outcome: 'posted' }, 'abc123', 'cli'); + }); + + test('the bot-token ack path also forwards its declared consumer', async () => { + mockUserFindById.mockReturnValue({ + lean: () => Promise.resolve({ + isBot: true, + username: 'pixel', + botMetadata: { agentName: 'pixel', instanceId: 'default' }, + }), + }); + + const res = await request(app) + .post(BOT_ACK) + .set('x-commonly-client', 'cli') + .send({ deliveryId: 'abc123' }); + + expect(res.status).toBe(200); + expect(mockAcknowledge).toHaveBeenCalledWith('evt-1', 'pixel', 'default', null, 'abc123', 'cli'); + }); + + test('a superseded delivery gets 409, not a cheerful 200', async () => { + mockAcknowledge.mockResolvedValue(null); + mockIsSuperseded.mockResolvedValue(true); + + const res = await request(app).post(ACK).send({ deliveryId: 'stale' }); + + expect(res.status).toBe(409); + expect(res.body.code).toBe('stale_delivery'); + }); + + test('a vanished event is still idempotent success — not every null is a refusal', async () => { + mockAcknowledge.mockResolvedValue(null); + mockIsSuperseded.mockResolvedValue(false); + + const res = await request(app).post(ACK).send({ deliveryId: 'whatever' }); + + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + }); + + test('Phase A: a nonce-less ack is accepted, and the service is told so', async () => { + const res = await request(app).post(ACK).send({ result: { outcome: 'acknowledged' } }); + + expect(res.status).toBe(200); + expect(mockAcknowledge).toHaveBeenCalledWith(...[ + 'evt-1', + 'pixel', + 'default', + { outcome: 'acknowledged' }, + null, + 'unknown', + ]); + expect(mockIsRequired).toHaveBeenCalledWith('unknown'); + }); + + test('Phase B: a nonce-less ack is refused loudly, before the service is called', async () => { + mockIsRequired.mockReturnValue(true); + + const res = await request(app) + .post(ACK) + .set('x-commonly-client', 'cli') + .send({ result: { outcome: 'acknowledged' } }); + + expect(res.status).toBe(400); + expect(res.body.code).toBe('delivery_id_required'); + expect(mockIsRequired).toHaveBeenCalledWith('cli'); + // The silent-success bug this pins: acknowledge() returning null under the + // flag looks exactly like "already gone", so the refusal has to happen + // where the caller can still be told about it. + expect(mockAcknowledge).not.toHaveBeenCalled(); + }); + + test('Phase B: a nonce-bearing ack is unaffected', async () => { + mockIsRequired.mockReturnValue(true); + + const res = await request(app).post(ACK).send({ deliveryId: 'abc123' }); + + expect(res.status).toBe(200); + expect(mockAcknowledge).toHaveBeenCalled(); + }); +}); diff --git a/backend/__tests__/unit/services/agentEventService.deliveryNonce.test.js b/backend/__tests__/unit/services/agentEventService.deliveryNonce.test.js new file mode 100644 index 0000000000..7544eaddff --- /dev/null +++ b/backend/__tests__/unit/services/agentEventService.deliveryNonce.test.js @@ -0,0 +1,516 @@ +// ADR-026 D6 — the delivery nonce, pinned against mongodb-memory-server. +// +// The race this exists for, from the code as it stood: `acknowledge` matched +// on {_id, agentName, instanceId, status ∈ {pending, delivered}} and nothing +// identifying WHICH delivery. So: +// +// 1. child A claims E → delivered, attempts 1 +// 2. A hangs; garbageCollect requeues E → pending, deliveredAt null +// 3. child B claims E → attempts 2, starts the turn +// 4. A wakes and acks → E goes terminal on A's delivery +// while B is mid-turn +// +// Both children ran the turn, and the lastSeenRevision bump used A's snapshot +// rather than B's. Daemon restart/upgrade (ADR-026) makes step 2-4 routine +// rather than exotic, which is why this lands before the supervisor does. +// +// The suite is written as a MUTATION test: delete the `deliveryNonce` clause +// from acknowledge's filter and `refuses the superseded child's late ack` +// flips to accepted. The revision assertion is the subtler half — a fix that +// rejects the stale ack but leaves A's memoryRevisionAtDelivery in place +// still advances the agent's read-checkpoint to the wrong snapshot. +const { MongoMemoryServer } = require('mongodb-memory-server'); +const mongoose = require('mongoose'); + +jest.mock('../../../services/agentMemoryService', () => ({ + buildMemoryDigestBundle: jest.fn(() => ({})), +})); +jest.mock('../../../services/agentProvisionerService', () => ({ + getAgentSessionSizes: jest.fn(), + clearAgentRuntimeSessions: jest.fn(), + restartAgentRuntime: jest.fn(), + resolveOpenClawAccountId: jest.fn(() => 'acct'), +})); +jest.mock('../../../services/agentIdentityService', () => ({ + getAgentTypeConfig: jest.fn(() => ({ runtime: 'moltbot' })), +})); +jest.mock('../../../services/nativeRuntimeService', () => ({ runAgent: jest.fn() })); + +let mongod; +let AgentEvent; +let AgentMemory; +let AgentEventService; + +const AGENT = 'pixel'; +const INSTANCE = 'default'; +const POD_ID = new mongoose.Types.ObjectId(); + +beforeAll(async () => { + mongod = await MongoMemoryServer.create(); + await mongoose.connect(mongod.getUri()); + AgentEvent = require('../../../models/AgentEvent'); + AgentMemory = require('../../../models/AgentMemory'); + AgentEventService = require('../../../services/agentEventService'); +}); + +afterAll(async () => { + await mongoose.disconnect(); + await mongod.stop(); +}); + +beforeEach(async () => { + await AgentEvent.deleteMany({}); + await AgentMemory.deleteMany({}); +}); + +const seedEvent = () => AgentEvent.create({ + agentName: AGENT, + instanceId: INSTANCE, + podId: POD_ID, + type: 'chat.mention', + payload: { text: 'hello' }, + status: 'pending', +}); + +// One claim through the real code path, returning the nonce the driver would +// have been handed in its payload. +const claim = async () => { + const [delivered] = await AgentEventService.list({ + agentName: AGENT, + instanceId: INSTANCE, + limit: 5, + }); + return delivered; +}; + +// Age the delivery past the requeue threshold and run the real pass, rather +// than hand-writing `status: 'pending'` — the point is that the REQUEUE +// invalidates, so a test that sets the field itself would pass against a +// requeue that forgot to. +const requeue = async (eventId) => { + await AgentEvent.updateOne( + { _id: eventId }, + { $set: { deliveredAt: new Date(Date.now() - 60 * 60 * 1000) } }, + ); + await AgentEventService.garbageCollect({ requeueDeliveredMinutes: 10 }); +}; + +describe('delivery nonce', () => { + test('the claim mints a nonce and hands it to the driver as deliveryId', async () => { + const event = await seedEvent(); + const delivered = await claim(); + + expect(delivered).toBeTruthy(); + expect(delivered.payload.deliveryId).toEqual(expect.any(String)); + expect(delivered.payload.deliveryId).toHaveLength(32); + + const stored = await AgentEvent.findById(event._id).lean(); + expect(stored.status).toBe('delivered'); + expect(stored.deliveryNonce).toBe(delivered.payload.deliveryId); + }); + + test('the requeue clears it, and the next claim mints a different one', async () => { + const event = await seedEvent(); + const first = await claim(); + + await requeue(event._id); + + const requeued = await AgentEvent.findById(event._id).lean(); + expect(requeued.status).toBe('pending'); + // The invalidation itself. Without this line in garbageCollect, the stale + // child's nonce still matches and the whole gate is decorative. + expect(requeued.deliveryNonce).toBeNull(); + + const second = await claim(); + expect(second.payload.deliveryId).not.toBe(first.payload.deliveryId); + }); + + test('refuses the superseded child\'s late ack, and keeps the new delivery live', async () => { + const event = await seedEvent(); + const childA = await claim(); + + await requeue(event._id); + const childB = await claim(); + expect(childB.payload.deliveryId).not.toBe(childA.payload.deliveryId); + + const staleAck = await AgentEventService.acknowledge( + event._id, + AGENT, + INSTANCE, + { outcome: 'no_action' }, + childA.payload.deliveryId, + ); + + expect(staleAck).toBeNull(); + const afterStale = await AgentEvent.findById(event._id).lean(); + // B is still mid-turn: its delivery must survive A's ack untouched. + expect(afterStale.status).toBe('delivered'); + expect(afterStale.deliveryNonce).toBe(childB.payload.deliveryId); + // A's outcome must not be recorded as the event's result. + expect(afterStale.delivery?.outcome).toBeUndefined(); + }); + + test('the holding child\'s ack lands, and advances the checkpoint to ITS snapshot', async () => { + await AgentMemory.create({ + agentName: AGENT, + instanceId: INSTANCE, + revision: 7, + lastSeenRevision: 1, + }); + + const event = await seedEvent(); + const childA = await claim(); + + // Memory moves on between the two deliveries, so A's snapshot and B's are + // distinguishable — that difference is what the last assertion reads. + await AgentMemory.updateOne( + { agentName: AGENT, instanceId: INSTANCE }, + { $set: { revision: 9 } }, + ); + + await requeue(event._id); + const childB = await claim(); + + const acked = await AgentEventService.acknowledge( + event._id, + AGENT, + INSTANCE, + { outcome: 'posted' }, + childB.payload.deliveryId, + ); + + expect(acked).toBeTruthy(); + const stored = await AgentEvent.findById(event._id).lean(); + expect(stored.status).toBe('acked'); + expect(stored.delivery.outcome).toBe('posted'); + // Terminal: nothing left for a later ack to match. + expect(stored.deliveryNonce).toBeNull(); + + const memory = await AgentMemory.findOne({ agentName: AGENT, instanceId: INSTANCE }).lean(); + // 9, not 7 — a fix that only rejects the stale ack would leave the + // checkpoint on A's snapshot and corrupt the agent's memory read window. + expect(memory.lastSeenRevision).toBe(9); + expect(childA.payload.deliveryId).not.toBe(childB.payload.deliveryId); + }); + + test('an ack with no deliveryId still lands — the pre-D6 fleet keeps working', async () => { + const event = await seedEvent(); + await claim(); + + const acked = await AgentEventService.acknowledge( + event._id, + AGENT, + INSTANCE, + { outcome: 'acknowledged' }, + ); + + expect(acked).toBeTruthy(); + expect((await AgentEvent.findById(event._id).lean()).status).toBe('acked'); + }); + + test('an unguessable nonce also stops acking a delivery you never received', async () => { + const event = await seedEvent(); + await claim(); + + const forged = await AgentEventService.acknowledge( + event._id, + AGENT, + INSTANCE, + { outcome: 'no_action' }, + 'f'.repeat(32), + ); + + expect(forged).toBeNull(); + expect((await AgentEvent.findById(event._id).lean()).status).toBe('delivered'); + }); +}); + +describe('isSupersededDelivery — telling "replaced" apart from "gone"', () => { + test('true when the event is live under a newer nonce', async () => { + const event = await seedEvent(); + const childA = await claim(); + await requeue(event._id); + await claim(); + + await expect(AgentEventService.isSupersededDelivery( + event._id, AGENT, INSTANCE, childA.payload.deliveryId, + )).resolves.toBe(true); + }); + + test('false once the event is terminal — that is idempotent success, not supersession', async () => { + const event = await seedEvent(); + const child = await claim(); + await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'posted' }, child.payload.deliveryId, + ); + + await expect(AgentEventService.isSupersededDelivery( + event._id, AGENT, INSTANCE, child.payload.deliveryId, + )).resolves.toBe(false); + }); + + test('false for a caller that presented no nonce at all', async () => { + const event = await seedEvent(); + await claim(); + + await expect(AgentEventService.isSupersededDelivery( + event._id, AGENT, INSTANCE, null, + )).resolves.toBe(false); + }); +}); + +describe('markPosted annotates the delivery in flight, it does not start a new one', () => { + test('an already-acked event is not returned to delivered', async () => { + const event = await seedEvent(); + const child = await claim(); + await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'no_action' }, child.payload.deliveryId, + ); + + await AgentEventService.markPosted(String(event._id), AGENT, INSTANCE, { messageId: 'm1' }); + + const stored = await AgentEvent.findById(event._id).lean(); + // Without the status gate this row went back to 'delivered' with a fresh + // deliveredAt — straight into the requeue population, re-delivering work + // that was already finished. + expect(stored.status).toBe('acked'); + }); + + test('a requeued event is not flipped back to delivered by a stale post', async () => { + const event = await seedEvent(); + await claim(); + await requeue(event._id); + + // The stale child posts after losing its delivery. Under the first, + // looser gate ($ne: 'acked') this flipped pending → delivered with a null + // nonce, and the next claim then saw nothing — the event was hidden for + // another full requeue window by a child that no longer owned it. + await AgentEventService.markPosted(String(event._id), AGENT, INSTANCE, { messageId: 'm-stale' }); + + const stored = await AgentEvent.findById(event._id).lean(); + expect(stored.status).toBe('pending'); + const replacement = await claim(); + expect(replacement).toBeTruthy(); + expect(replacement.payload.deliveryId).toEqual(expect.any(String)); + }); + + test('a failed event retired by the cap is not resurrected by a post', async () => { + const event = await seedEvent(); + await AgentEvent.updateOne( + { _id: event._id }, + { $set: { status: 'failed', error: 'retired', deliveryNonce: null } }, + ); + + await AgentEventService.markPosted(String(event._id), AGENT, INSTANCE, { messageId: 'm-late' }); + + expect((await AgentEvent.findById(event._id).lean()).status).toBe('failed'); + }); + + test('it leaves the holder\'s nonce intact, so the holder can still ack', async () => { + const event = await seedEvent(); + const child = await claim(); + + await AgentEventService.markPosted(String(event._id), AGENT, INSTANCE, { messageId: 'm1' }); + + const stored = await AgentEvent.findById(event._id).lean(); + expect(stored.deliveryNonce).toBe(child.payload.deliveryId); + + const acked = await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'posted' }, child.payload.deliveryId, + ); + expect(acked).toBeTruthy(); + }); +}); + +describe('the Phase A migration counter', () => { + test('counts nonce-bearing and nonce-less acks apart', async () => { + const before = AgentEventService.getAckNonceStats(); + + const withNonce = await seedEvent(); + const child = await claim(); + await AgentEventService.acknowledge( + withNonce._id, AGENT, INSTANCE, { outcome: 'no_action' }, child.payload.deliveryId, + ); + + const withoutNonce = await seedEvent(); + await claim(); + await AgentEventService.acknowledge( + withoutNonce._id, AGENT, INSTANCE, { outcome: 'no_action' }, + ); + + const after = AgentEventService.getAckNonceStats(); + expect(after.withNonce - before.withNonce).toBe(1); + // This is the number Phase B is gated on. A counter nobody can read is a + // migration plan with no exit condition. + expect(after.withoutNonce - before.withoutNonce).toBe(1); + }); +}); + +describe('the native path carries its own delivery nonce', () => { + const { runAgent } = require('../../../services/nativeRuntimeService'); + const { AgentInstallation } = require('../../../models/AgentRegistry'); + + // enqueue() routes to the native runtime when the installation says so; the + // event is then born 'delivered' and the settle handlers are its ack. + const nativeInstall = { + _id: new mongoose.Types.ObjectId(), + agentName: AGENT, + instanceId: INSTANCE, + podId: POD_ID, + status: 'active', + config: { runtime: { runtimeType: 'native' } }, + }; + + afterEach(() => { jest.restoreAllMocks(); }); + + test('a native run that outlives the requeue does not terminate its replacement', async () => { + let settle; + runAgent.mockImplementation(() => new Promise((resolve) => { settle = resolve; })); + jest.spyOn(AgentInstallation, 'findOne').mockReturnValue({ + lean: () => Promise.resolve(nativeInstall), + }); + + const event = await AgentEventService.enqueue({ + agentName: AGENT, + instanceId: INSTANCE, + podId: POD_ID, + type: 'chat.mention', + payload: { text: 'hi' }, + }); + + const born = await AgentEvent.findById(event._id).lean(); + expect(born.status).toBe('delivered'); + // Minting is not enough — the settle handler has to CARRY it. + expect(born.deliveryNonce).toEqual(expect.any(String)); + + await requeue(event._id); + const replacement = await claim(); + expect(replacement.payload.deliveryId).not.toBe(born.deliveryNonce); + + // The slow native run finally finishes and acks. + settle('done'); + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + + const after = await AgentEvent.findById(event._id).lean(); + expect(after.status).toBe('delivered'); + expect(after.deliveryNonce).toBe(replacement.payload.deliveryId); + }); +}); + +describe('recordFailure is as terminal as an ack, and gated the same way', () => { + test('a stale runner\'s failure does not retire the replacement\'s delivery', async () => { + const event = await seedEvent(); + const childA = await claim(); + await requeue(event._id); + const childB = await claim(); + + await AgentEventService.recordFailure( + String(event._id), AGENT, INSTANCE, 'stale runner blew up', childA.payload.deliveryId, + ); + + const stored = await AgentEvent.findById(event._id).lean(); + expect(stored.status).toBe('delivered'); + expect(stored.deliveryNonce).toBe(childB.payload.deliveryId); + }); +}); + +describe('recordFailure will not overwrite a terminal event', () => { + test('a late nonce-less failure leaves an acked event acked', async () => { + const event = await seedEvent(); + const child = await claim(); + await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'posted' }, child.payload.deliveryId, + ); + + await AgentEventService.recordFailure(String(event._id), AGENT, INSTANCE, 'late boom'); + + const stored = await AgentEvent.findById(event._id).lean(); + expect(stored.status).toBe('acked'); + expect(stored.error).toBeUndefined(); + }); +}); + +describe('Phase B has a switch, so the compatibility mode terminates', () => { + afterEach(() => { delete process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE; }); + + test('with the flag set, a nonce-less ack is refused', async () => { + process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE = 'true'; + const event = await seedEvent(); + await claim(); + + const acked = await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'acknowledged' }, + ); + + expect(acked).toBeNull(); + expect((await AgentEvent.findById(event._id).lean()).status).toBe('delivered'); + }); + + test('with the flag set, a matching nonce still lands', async () => { + process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE = 'true'; + const event = await seedEvent(); + const child = await claim(); + + const acked = await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'acknowledged' }, child.payload.deliveryId, + ); + + expect(acked).toBeTruthy(); + }); +}); + +describe('Phase B flips per consumer, so a parked path cannot veto it', () => { + afterEach(() => { delete process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE; }); + + test('a listed consumer is enforced while an unlisted one keeps working', async () => { + process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE = 'ws,cli'; + + expect(AgentEventService.isDeliveryNonceRequired('ws')).toBe(true); + expect(AgentEventService.isDeliveryNonceRequired('cli')).toBe(true); + // The parked openclaw extension. It must not hold the flip hostage, and + // it must not break while it is parked. + expect(AgentEventService.isDeliveryNonceRequired('openclaw')).toBe(false); + expect(AgentEventService.isDeliveryNonceRequired()).toBe(false); + + const event = await seedEvent(); + await claim(); + const acked = await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'acknowledged' }, null, 'openclaw', + ); + expect(acked).toBeTruthy(); + }); + + test('a listed consumer acking nonce-less is refused', async () => { + process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE = 'ws,cli'; + const event = await seedEvent(); + await claim(); + + const acked = await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'acknowledged' }, null, 'cli', + ); + + expect(acked).toBeNull(); + expect((await AgentEvent.findById(event._id).lean()).status).toBe('delivered'); + }); + + test("`true` still means everyone — the global switch is not lost", async () => { + process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE = 'true'; + expect(AgentEventService.isDeliveryNonceRequired('openclaw')).toBe(true); + expect(AgentEventService.isDeliveryNonceRequired()).toBe(true); + }); + + test('the counter attributes nonce-less acks to the consumer that made them', async () => { + const before = AgentEventService.getAckNonceStats().withoutNonceByConsumer.openclaw || 0; + const event = await seedEvent(); + await claim(); + + await AgentEventService.acknowledge( + event._id, AGENT, INSTANCE, { outcome: 'acknowledged' }, null, 'openclaw', + ); + + const after = AgentEventService.getAckNonceStats().withoutNonceByConsumer.openclaw || 0; + // Naming the holdout is what makes a per-consumer flip decidable. + expect(after - before).toBe(1); + }); +}); diff --git a/backend/__tests__/unit/services/agentEventService.lifecycle.test.js b/backend/__tests__/unit/services/agentEventService.lifecycle.test.js index be359ba878..e53df7b5a8 100644 --- a/backend/__tests__/unit/services/agentEventService.lifecycle.test.js +++ b/backend/__tests__/unit/services/agentEventService.lifecycle.test.js @@ -255,7 +255,16 @@ describe('garbageCollect: the requeue predicate and its cap', () => { // that only checked the surviving keys would pass with it still there. expect(Object.keys(requeue.filter)).not.toContain('ackedAt'); expect(requeue.filter).toEqual(expect.objectContaining({ status: 'delivered' })); - expect(requeue.update.$set).toEqual({ status: 'pending', deliveredAt: null }); + expect(requeue.update.$set).toEqual({ + status: 'pending', + deliveredAt: null, + // ADR-026 D6: the requeue clearing the nonce IS the invalidation — it is + // what stops the superseded child's late ack from terminating the + // replacement's delivery. Asserted here as well as in the D6 suite + // because this is the test that reads the whole $set exhaustively, so a + // future edit that drops the field fails here first. + deliveryNonce: null, + }); }); test('a second pass retires cap-exhausted events to the terminal failed state', async () => { diff --git a/backend/__tests__/unit/services/agentWebSocketService.ack.test.js b/backend/__tests__/unit/services/agentWebSocketService.ack.test.js new file mode 100644 index 0000000000..d8e1e8448e --- /dev/null +++ b/backend/__tests__/unit/services/agentWebSocketService.ack.test.js @@ -0,0 +1,202 @@ +/** + * ADR-026 D6 on the WebSocket ack surface. + * + * This handler previously called acknowledge() with three arguments and + * emitted `ack:success` regardless of the return value — the same lie the two + * HTTP ack routes grew a 400 to prevent, on a surface that is live + * (server.ts init(io)). Under Phase B every WS ack would take the + * `return null` branch in acknowledge, the socket would report success, and + * the event would quietly requeue and redeliver. + * + * It also matters for the migration's exit condition: while any WS driver + * acks nonce-less, `ackNonceStats.withoutNonce` can never reach zero, and + * that number is the stated gate for flipping the flag. + */ +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn(), findOne: jest.fn(), updateOne: jest.fn() }, +})); +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../models/User', () => ({ findOne: jest.fn(), updateOne: jest.fn() })); +jest.mock('../../../models/AgentCredential', () => ({ + findOne: jest.fn().mockResolvedValue(null), + findById: jest.fn().mockResolvedValue(null), +})); +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); + +const mockAcknowledge = jest.fn(); +const mockIsSuperseded = jest.fn(); +const mockIsRequired = jest.fn(); +jest.mock('../../../services/agentEventService', () => ({ + acknowledge: (...a) => mockAcknowledge(...a), + isSupersededDelivery: (...a) => mockIsSuperseded(...a), + isDeliveryNonceRequired: (...a) => mockIsRequired(...a), + list: jest.fn().mockResolvedValue([]), +})); + +const { AgentInstallation } = require('../../../models/AgentRegistry'); +const Pod = require('../../../models/Pod'); +const agentWebSocketService = require('../../../services/agentWebSocketService'); + +// Drive the real registration path: init(io) → namespace.on('connection') → +// the socket's own 'ack' listener. Testing the handler any other way would +// test a copy of it. +const wireSocket = (socketOptions = {}) => { + const handlers = {}; + const emitted = []; + const emit = jest.fn((event, body) => emitted.push({ event, body })); + const socket = { + agentKey: 'pixel:default', + agentName: 'pixel', + instanceId: 'default', + agentUserId: null, + subscribedPods: new Set(), + join: jest.fn(), + leave: jest.fn(), + on: (event, fn) => { handlers[event] = fn; }, + emit, + ...socketOptions, + }; + + let connectionHandler; + agentWebSocketService.init({ + of: () => ({ + use: jest.fn(), + on: (event, fn) => { if (event === 'connection') connectionHandler = fn; }, + }), + }); + connectionHandler(socket); + return { handlers, emitted, socket }; +}; + +describe("the WS 'ack' handler tells the truth about refusals", () => { + beforeEach(() => { + jest.clearAllMocks(); + AgentInstallation.find.mockReturnValue({ select: () => ({ lean: () => Promise.resolve([]) }) }); + Pod.find.mockReturnValue({ select: () => ({ lean: () => Promise.resolve([]) }) }); + mockIsRequired.mockReturnValue(false); + mockIsSuperseded.mockResolvedValue(false); + mockAcknowledge.mockResolvedValue({ _id: 'evt-1', status: 'acked' }); + }); + + test('forwards the deliveryId and the ws consumer', async () => { + const { handlers, emitted } = wireSocket(); + + await handlers.ack({ eventId: 'evt-1', deliveryId: 'abc123' }); + + expect(mockAcknowledge).toHaveBeenCalledWith('evt-1', 'pixel', 'default', null, 'abc123', 'ws'); + expect(emitted.map((e) => e.event)).toContain('ack:success'); + }); + + test('Phase B: a nonce-less ack is refused, and the socket is told', async () => { + mockIsRequired.mockReturnValue(true); + const { handlers, emitted } = wireSocket(); + + await handlers.ack({ eventId: 'evt-1' }); + + // The bug this pins: emitting ack:success here told the driver it had + // acked while the event rolled into the requeue unhandled. + expect(mockAcknowledge).not.toHaveBeenCalled(); + expect(mockIsRequired).toHaveBeenCalledWith('ws'); + expect(emitted).toContainEqual( + expect.objectContaining({ event: 'ack:error', body: expect.objectContaining({ code: 'delivery_id_required' }) }), + ); + }); + + test('a superseded delivery reports an error, not success', async () => { + mockAcknowledge.mockResolvedValue(null); + mockIsSuperseded.mockResolvedValue(true); + const { handlers, emitted } = wireSocket(); + + await handlers.ack({ eventId: 'evt-1', deliveryId: 'stale' }); + + expect(emitted).toContainEqual( + expect.objectContaining({ event: 'ack:error', body: expect.objectContaining({ code: 'stale_delivery' }) }), + ); + }); + + test('a vanished event stays idempotent success', async () => { + mockAcknowledge.mockResolvedValue(null); + mockIsSuperseded.mockResolvedValue(false); + const { handlers, emitted } = wireSocket(); + + await handlers.ack({ eventId: 'evt-1', deliveryId: 'gone' }); + + expect(emitted.map((e) => e.event)).toContain('ack:success'); + }); + + test('Phase A is unchanged for a nonce-less driver', async () => { + const { handlers, emitted } = wireSocket(); + + await handlers.ack({ eventId: 'evt-1' }); + + expect(mockAcknowledge).toHaveBeenCalledWith('evt-1', 'pixel', 'default', null, null, 'ws'); + expect(emitted.map((e) => e.event)).toContain('ack:success'); + }); +}); + +describe('the WS event push path claims before it delivers', () => { + beforeEach(() => { + jest.clearAllMocks(); + AgentInstallation.find.mockReturnValue({ select: () => ({ lean: () => Promise.resolve([]) }) }); + Pod.find.mockReturnValue({ select: () => ({ lean: () => Promise.resolve([]) }) }); + }); + + test('a pending event wakes its target through replay, not a raw namespace broadcast', () => { + const { socket } = wireSocket(); + const replay = jest.spyOn(agentWebSocketService, 'replayPendingEvents').mockResolvedValue(); + socket.emit.mockClear(); + + const delivered = agentWebSocketService.pushEvent({ + _id: 'evt-pending', + agentName: 'pixel', + instanceId: 'default', + podId: 'pod-1', + type: 'message.created', + }); + + // replayPendingEvents calls list(), whose conditional claim mints the + // nonce and emits that claimed event. This pairing is the D6 boundary. + expect(delivered).toBe(true); + expect(replay).toHaveBeenCalledWith(socket); + expect(socket.emit).not.toHaveBeenCalledWith('event', expect.anything()); + }); + + test('a native event that was born claimed uses the same payload nonce shape as replay', () => { + const { socket } = wireSocket(); + const replay = jest.spyOn(agentWebSocketService, 'replayPendingEvents').mockResolvedValue(); + socket.emit.mockClear(); + const event = { + _id: 'evt-native', + agentName: 'pixel', + instanceId: 'default', + deliveryId: 'native-delivery', + }; + + expect(agentWebSocketService.pushEvent(event)).toBe(true); + expect(socket.emit).toHaveBeenCalledWith('event', { + _id: 'evt-native', + agentName: 'pixel', + instanceId: 'default', + payload: { deliveryId: 'native-delivery' }, + }); + expect(replay).not.toHaveBeenCalled(); + }); + + test('an old disconnect cannot evict a replacement socket for the same agent', () => { + const first = wireSocket(); + const second = wireSocket(); + const replay = jest.spyOn(agentWebSocketService, 'replayPendingEvents').mockResolvedValue(); + first.socket.emit.mockClear(); + second.socket.emit.mockClear(); + + first.handlers.disconnect('ping timeout'); + + expect(agentWebSocketService.pushEvent({ + _id: 'evt-after-reconnect', + agentName: 'pixel', + instanceId: 'default', + })).toBe(true); + expect(replay).toHaveBeenCalledWith(second.socket); + expect(replay).not.toHaveBeenCalledWith(first.socket); + }); +}); diff --git a/backend/models/AgentEvent.ts b/backend/models/AgentEvent.ts index 519a3665f1..888a43f755 100644 --- a/backend/models/AgentEvent.ts +++ b/backend/models/AgentEvent.ts @@ -66,6 +66,12 @@ export interface IAgentEvent extends Document { // can bump lastSeenRevision exactly once per event without trusting // the client to echo it. `null` for events fetched before §3 shipped. memoryRevisionAtDelivery?: number | null; + // ADR-026 D6: identifies WHICH delivery is outstanding. Minted at every + // pending → delivered claim, echoed to the driver as `deliveryId`, and + // required to match on ack. Cleared by the requeue, which is what makes a + // superseded child's late ack fail instead of terminating the new child's + // delivery. `null` for events claimed before D6 shipped. + deliveryNonce?: string | null; createdAt: Date; updatedAt: Date; } @@ -82,6 +88,7 @@ const AgentEventSchema = new Schema( deliveredAt: Date, error: String, memoryRevisionAtDelivery: { type: Number, default: null }, + deliveryNonce: { type: String, default: null }, delivery: { outcome: { type: String, diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index 4023ef94e3..68ff6b31d7 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -1101,7 +1101,40 @@ router.post('/bot/events/:id/ack', auth, requireApiTokenScopes(['agent:events:ac return res.status(403).json({ message: 'Agent token does not match bot user' }); } const delivery = req.body?.result || req.body?.delivery || null; - await AgentEventService.acknowledge(req.params.id, resolvedAgentName, instanceId, delivery); + const deliveryId = req.body?.deliveryId || null; + // Which consumer is acking. Phase B is keyed on this rather than flipped + // globally, so a migrated client can enforce while a parked one (the + // openclaw extension, parked 2026-08-20) keeps working. Clients declare + // themselves with `x-commonly-client`; anything that does not is + // 'unknown', which is never enforced unless the operator sets `true`/`*`. + const ackConsumer = String(req.header?.('x-commonly-client') || '').trim().toLowerCase() || 'unknown'; + // Phase B: refuse rather than accept-and-say-nothing. acknowledge() + // returns null for a refused nonce-less ack, which is indistinguishable + // from "already gone", so answering 200 here would tell the driver it + // acked while the event rolled into the requeue unhandled. + if (!deliveryId && AgentEventService.isDeliveryNonceRequired(ackConsumer)) { + return res.status(400).json({ + code: 'delivery_id_required', + message: 'This instance requires the deliveryId from the event payload on ack', + }); + } + const acked = await AgentEventService.acknowledge( + req.params.id, + resolvedAgentName, + instanceId, + delivery, + deliveryId, + ackConsumer, + ); + // ADR-026 D6: only a caller that presented a deliveryId can be told it was + // superseded, so pre-D6 drivers see byte-identical behaviour. 409, not 404 + // — "the event is gone" is idempotent success, "you were replaced" means + // stop working. + if (!acked && await AgentEventService.isSupersededDelivery( + req.params.id, resolvedAgentName, instanceId, deliveryId, + )) { + return res.status(409).json({ code: 'stale_delivery', message: 'This delivery was superseded by a requeue' }); + } return res.json({ success: true }); } catch (error: any) { @@ -1125,12 +1158,37 @@ router.post('/events/:id/ack', agentRuntimeAuth, async (req: any, res: any) => { return res.status(403).json({ message: 'Agent token not authorized for events' }); } const delivery = req.body?.result || req.body?.delivery || null; - await AgentEventService.acknowledge( + const deliveryId = req.body?.deliveryId || null; + // Which consumer is acking. Phase B is keyed on this rather than flipped + // globally, so a migrated client can enforce while a parked one (the + // openclaw extension, parked 2026-08-20) keeps working. Clients declare + // themselves with `x-commonly-client`; anything that does not is + // 'unknown', which is never enforced unless the operator sets `true`/`*`. + const ackConsumer = String(req.header?.('x-commonly-client') || '').trim().toLowerCase() || 'unknown'; + // Phase B: refuse rather than accept-and-say-nothing. acknowledge() + // returns null for a refused nonce-less ack, which is indistinguishable + // from "already gone", so answering 200 here would tell the driver it + // acked while the event rolled into the requeue unhandled. + if (!deliveryId && AgentEventService.isDeliveryNonceRequired(ackConsumer)) { + return res.status(400).json({ + code: 'delivery_id_required', + message: 'This instance requires the deliveryId from the event payload on ack', + }); + } + const acked = await AgentEventService.acknowledge( req.params.id, agentName, instanceId, delivery, + deliveryId, + ackConsumer, ); + // See the /bot/events ack above: 409 only for nonce-presenting callers. + if (!acked && await AgentEventService.isSupersededDelivery( + req.params.id, agentName, instanceId, deliveryId, + )) { + return res.status(409).json({ code: 'stale_delivery', message: 'This delivery was superseded by a requeue' }); + } return res.json({ success: true }); } catch (error: any) { console.error('Error acknowledging agent event:', error); diff --git a/backend/services/agentEventService.ts b/backend/services/agentEventService.ts index 8d0bc76316..1028c9ecda 100644 --- a/backend/services/agentEventService.ts +++ b/backend/services/agentEventService.ts @@ -31,6 +31,68 @@ const { // into an accidental count of historical board churn. const TASK_UPDATE_CUE_MAX_CURSOR_AGE_MS = 24 * 60 * 60 * 1000; +// ADR-026 D6. Every CLAIM mints one — the list() claim and the +// native-runtime create, which is born 'delivered'. The value comes from one +// function rather than two literals that have to agree forever; the design +// doc listed three writers of `status: 'delivered'`, and reading the third +// (markPosted) closely corrected that: it annotates the delivery already in +// flight rather than starting a new one, so minting there would invalidate +// the nonce held by the very child that just posted. +// +// Opaque and unguessable on purpose: a client that never received a delivery +// cannot fabricate its nonce, which also closes a smaller hole the race work +// exposed — `acknowledge` authorizes on the token's agentName/instanceId, so +// before this any holder of an agent's token could ack ANY of that agent's +// events by id, delivered to it or not. +const mintDeliveryNonce = (): string => crypto.randomBytes(16).toString('hex'); + +// The migration on acknowledge() runs in two phases, and BOTH are in this +// build so the compatibility mode has an end rather than an intention. +// +// Phase A (default): a presented nonce must match; an absent one is accepted +// and counted. Phase B: an absent nonce is refused. +// +// **Phase B flips PER CONSUMER, not globally** (Sam, 2026-08-30). The first +// draft gated the flip on `withoutNonce` reaching zero across everything, +// which hands a veto to whichever consumer is slowest — and one of them, the +// openclaw extension, has been parked since 2026-08-20. A parked runtime +// cannot be the long pole on a live kernel change, so enforcement is keyed on +// HOW THE ACK ARRIVED: +// +// AGENT_EVENT_REQUIRE_DELIVERY_NONCE=ws,cli,hosted → those three enforce +// AGENT_EVENT_REQUIRE_DELIVERY_NONCE=true (or `*`) → everything enforces +// unset → Phase A everywhere +// +// A consumer key that is not listed stays on Phase A indefinitely, which is +// the point: migrated paths get the guarantee now, unmigrated ones keep +// working, and neither waits on the other. +// +// Read the counter precisely (review on #1347): it counts ack CALLS, not +// distinct deliveries — a retry against a vanished event still increments — +// and it is per-process and resets on restart. With multiple replicas, "zero +// for this consumer" means zero on EVERY replica since its last restart. +const ackNonceStats: { + withNonce: number; + withoutNonce: number; + withoutNonceByConsumer: Record; +} = { withNonce: 0, withoutNonce: 0, withoutNonceByConsumer: {} }; + +const enforcedAckConsumers = (): Set => new Set( + String(process.env.AGENT_EVENT_REQUIRE_DELIVERY_NONCE || '') + .split(',') + .map((entry) => entry.trim().toLowerCase()) + .filter(Boolean), +); + +// No consumer argument = "is it required for everyone?" — which is what the +// service-internal check asks, since acknowledge() cannot see the transport. +// The per-consumer refusal happens at the ack sites, where the key is known. +const requireDeliveryNonce = (consumer?: string): boolean => { + const enforced = enforcedAckConsumers(); + if (enforced.has('true') || enforced.has('*')) return true; + return consumer ? enforced.has(consumer.toLowerCase()) : false; +}; + interface EventDoc { _id?: unknown; type?: string; @@ -43,6 +105,7 @@ interface EventDoc { attempts?: number; delivery?: DeliveryMeta; memoryRevisionAtDelivery?: number | null; + deliveryNonce?: string | null; } interface InstallationDoc { @@ -589,6 +652,26 @@ class AgentEventService { }; } + // Phase B on? The routes need this BEFORE calling acknowledge: with the + // flag set, a nonce-less ack returns null, and null is indistinguishable + // from "already gone" — so the route would answer 200 and the driver would + // believe it acked while the event rolled into the requeue with no signal + // (found in review). A refused ack has to say so. + static isDeliveryNonceRequired(consumer?: string): boolean { + return requireDeliveryNonce(consumer); + } + + // Reads the Phase A migration counter. The flip to a required nonce is + // gated on `withoutNonce` being zero at the consumers, so the number has to + // be reachable by something other than a grep of the logs. + static getAckNonceStats(): { + withNonce: number; + withoutNonce: number; + withoutNonceByConsumer: Record; + } { + return { ...ackNonceStats, withoutNonceByConsumer: { ...ackNonceStats.withoutNonceByConsumer } }; + } + static logEventLifecycle(action: string, details: Record = {}): void { const parts = [ `[agent-event] ${action}`, @@ -650,7 +733,12 @@ class AgentEventService { $or: [{ attempts: { $lt: attemptCap } }, { attempts: { $exists: false } }], }, { - $set: { status: 'pending', deliveredAt: null }, + // ADR-026 D6: clearing the nonce IS the invalidation. The child + // whose delivery this was can no longer ack it, whether or not a + // replacement has claimed yet — so a supervisor restart can no + // longer let a stale child terminate the new child's delivery + // mid-turn. + $set: { status: 'pending', deliveredAt: null, deliveryNonce: null }, }, ) as { modifiedCount?: number }; requeuedDelivered = requeueResult?.modifiedCount || 0; @@ -658,6 +746,20 @@ class AgentEventService { console.log(`[agent-event] requeued ${requeuedDelivered} stuck 'delivered' events older than ${requeueDeliveredMinutes}min`); } + // D6 rollout coverage, on the pass that already runs on a schedule — + // no new surface, and it lands next to the requeue it is about. This is + // the number Phase B is gated on; `without=0` sustained past one requeue + // window is the signal to set AGENT_EVENT_REQUIRE_DELIVERY_NONCE. + console.log( + `[agent-event] ack deliveryId coverage (ack CALLS this process since restart, not distinct deliveries): ` + + `with=${ackNonceStats.withNonce} without=${ackNonceStats.withoutNonce} ` + // Naming the holdouts is the whole point of a per-consumer flip: the + // operator needs to see WHICH consumer is still nonce-less, not just + // that something is. + + `withoutBy=${JSON.stringify(ackNonceStats.withoutNonceByConsumer)} ` + + `enforced=${JSON.stringify([...enforcedAckConsumers()])}`, + ); + // Retire events that have exhausted the cap. This pass is what makes the // cap a bound rather than a leak: `attempts >= cap` fails the requeue // predicate, and a 'delivered' row is invisible to list(), so without a @@ -674,6 +776,7 @@ class AgentEventService { { $set: { status: 'failed', + deliveryNonce: null, error: `requeue cap exhausted after ${attemptCap} delivery attempts without an ack`, }, }, @@ -974,7 +1077,12 @@ class AgentEventService { // entered the pending queue ~10-20 min after creation, guaranteed. The // settle handlers below close it — success acks, failure records. ...(routedToNative - ? { status: 'delivered', deliveredAt: new Date(), attempts: 1 } + ? { + status: 'delivered', + deliveredAt: new Date(), + attempts: 1, + deliveryNonce: mintDeliveryNonce(), + } : {}), }) as EventDoc; @@ -983,6 +1091,12 @@ class AgentEventService { // eslint-disable-next-line global-require, @typescript-eslint/no-require-imports const { runAgent } = require('./nativeRuntimeService'); const eventIdStr = String((event._id as { toString?: () => string })?.toString?.() || ''); + // D6: the native path claims at creation, so THIS is its delivery + // nonce — and the settle handlers below are its ack. Without carrying + // it, a native run that outlives the requeue threshold settles + // nonce-less and terminates whatever replacement has since claimed the + // event: the D6 race, on the one path that never polls. + const nativeDeliveryId = event.deliveryNonce || null; // Fire-and-forget — callers of enqueue() must never block on the // loop. Errors are logged but never rethrown. // Settle the event on the run's actual outcome. nativeRuntimeService @@ -1007,6 +1121,8 @@ class AgentEventService { agentName, instanceId, { outcome: 'acknowledged', reason: 'native-runtime-completed' }, + nativeDeliveryId, + 'native', ).catch((ackErr: Error) => { console.warn('[native-runtime] ack after successful run failed:', ackErr?.message || ackErr); }), @@ -1020,6 +1136,7 @@ class AgentEventService { agentName, instanceId, `native runtime error: ${err?.message || String(err)}`, + nativeDeliveryId, ).catch(() => undefined); }, ); @@ -1052,6 +1169,13 @@ class AgentEventService { type: event.type, payload: event.payload, createdAt: event.createdAt, + // D6: present only for a native event, which is born 'delivered' and + // therefore already claimed. For every other route this push fires + // while the event is still 'pending' — no claim has happened, so no + // nonce exists and none can be invented here. The push is a wake + // signal, not a delivery: a WS driver still fetches through list(), + // and that claim is what hands it the deliveryId to ack with. + ...(event.deliveryNonce ? { deliveryId: event.deliveryNonce } : {}), }); } @@ -1170,6 +1294,9 @@ class AgentEventService { status: 'delivered', memoryRevisionAtDelivery: currentRevision, deliveredAt: new Date(), + // D6: this claim's identity. A requeue clears it, so the child + // this delivery belongs to is the only one whose ack can land. + deliveryNonce: mintDeliveryNonce(), }, $inc: { attempts: 1 }, }, @@ -1187,6 +1314,10 @@ class AgentEventService { const enrichedPayload: Record = { ...basePayload, ...digestBundle, + // ADR-026 D6: the driver echoes this back on ack. Additive — a driver + // that ignores it still acks successfully during the migration window + // described on acknowledge(). + ...(event?.deliveryNonce ? { deliveryId: event.deliveryNonce } : {}), }; if (messageId !== undefined && messageId !== null && typeof messageId !== 'string') { enrichedPayload.messageId = String(messageId); @@ -1208,6 +1339,8 @@ class AgentEventService { agentName: string, instanceId = 'default', delivery: DeliveryMeta | null = null, + deliveryId: unknown = null, + consumer: string | null = null, ): Promise { // Same NoSQL-injection guard as list() — coerce all caller-supplied query // inputs to plain primitives before they reach Mongo. @@ -1216,17 +1349,48 @@ class AgentEventService { const safeAgentName = String(agentName || '').toLowerCase(); const safeInstanceId = String(instanceId || 'default'); const normalizedDelivery = this.normalizeDeliveryMeta(delivery || {}); + // ADR-026 D6 — WHICH delivery is being acked. + // + // Until this existed the filter below identified an event but not a + // delivery, so after the 10-minute requeue a stale child's late ack + // matched the NEW child's delivery: both had already spawned, both ran + // the turn, and the lastSeenRevision bump used the stale child's + // snapshot. Daemon restart (ADR-026) makes that sequence routine. + // + // Migration, deliberately two-phase: every driver in the fleet acks + // without a nonce today — the openclaw extension, @commonlyai/mcp, + // cloud-codex, the webhook SDK, the wrapper CLI. Requiring it on day one + // breaks all of them at once. So a presented nonce MUST match, and an + // absent one is accepted and counted. Phase B flips `absent` to a refusal + // only once that counter is zero AT THE CONSUMERS — publishing a client + // is not deploying one (AX 34). + const safeDeliveryId = typeof deliveryId === 'string' && deliveryId ? deliveryId : null; + const safeConsumer = String(consumer || 'unknown').toLowerCase(); + if (safeDeliveryId) { + ackNonceStats.withNonce += 1; + } else { + ackNonceStats.withoutNonce += 1; + ackNonceStats.withoutNonceByConsumer[safeConsumer] = (ackNonceStats.withoutNonceByConsumer[safeConsumer] || 0) + 1; + console.warn( + `[agent-event] ack without deliveryId from ${safeAgentName}:${safeInstanceId} via ${safeConsumer} — pre-D6 driver`, + ); + // Per-consumer: an unlisted consumer stays on Phase A even while others + // enforce, so a parked path cannot hold the flip hostage. + if (requireDeliveryNonce(safeConsumer)) return null; + } const result = await AgentEvent.findOneAndUpdate( { _id: safeEventId, agentName: safeAgentName, instanceId: safeInstanceId, status: { $in: ['pending', 'delivered'] }, + ...(safeDeliveryId ? { deliveryNonce: safeDeliveryId } : {}), }, { $set: { status: 'acked', deliveredAt: new Date(), + deliveryNonce: null, ...(normalizedDelivery ? { delivery: normalizedDelivery } : {}), }, // No $inc here. `attempts` counts deliveries (incremented at the claim @@ -1304,6 +1468,30 @@ class AgentEventService { return result; } + // Did this ack lose to a requeue? Only meaningful when the caller presented + // a deliveryId and the gated update matched nothing: the event may be gone, + // already acked, or still live under a NEWER nonce. Only the last of those + // means "you were superseded — stop working", and a driver cannot act on + // that distinction unless we make it. + static async isSupersededDelivery( + eventId: unknown, + agentName: string, + instanceId: string, + deliveryId: unknown, + ): Promise { + const safeEventId = eventId != null ? String(eventId) : ''; + const safeDeliveryId = typeof deliveryId === 'string' && deliveryId ? deliveryId : null; + if (!safeEventId || !safeDeliveryId) return false; + const event = await AgentEvent.findOne({ + _id: safeEventId, + agentName: String(agentName || '').toLowerCase(), + instanceId: String(instanceId || 'default'), + }).select('status deliveryNonce').lean() as { status?: string; deliveryNonce?: string | null } | null; + if (!event) return false; + if (event.status === 'acked' || event.status === 'failed') return false; + return event.deliveryNonce !== safeDeliveryId; + } + static async markPosted( eventId: unknown, agentName: string, @@ -1324,8 +1512,33 @@ class AgentEventService { // construction, not by trust in the call site. const safeAgentName = typeof agentName === 'string' ? agentName.toLowerCase() : ''; const safeInstanceId = typeof instanceId === 'string' ? instanceId : 'default'; + // `status: 'delivered'` is load-bearing, not defensive. Ungated, this + // write returned an already-terminal event to 'delivered' with a fresh + // deliveredAt — exactly the requeue's target population (garbageCollect + // selects status 'delivered' by deliveredAt age), so posting a second + // message citing a completed event re-delivered it. Same class as the + // webhook duplicate-delivery bug described at deliverEventViaWebhook, on + // the path that fix did not cover. + // + // `$ne: 'acked'` was the first attempt and was too loose in two ways + // (found in review): a stale child posting after a requeue flipped + // 'pending' back to 'delivered' with a null nonce, hiding the event from + // list() for another full requeue window; and a 'failed' event retired by + // the attempt cap could be resurrected the same way. Annotating a + // delivery only makes sense while one is in flight, so the predicate is + // the positive state, not the absence of the terminal one. + // + // It deliberately does NOT mint a delivery nonce. This is an annotation + // of the delivery already in flight, not a new claim — minting here would + // invalidate the nonce held by the very child that just posted, and its + // ack would then be refused as stale. Only a claim mints. return AgentEvent.findOneAndUpdate( - { _id: eventIdStr, agentName: safeAgentName, instanceId: safeInstanceId }, + { + _id: eventIdStr, + agentName: safeAgentName, + instanceId: safeInstanceId, + status: 'delivered', + }, { $set: { status: 'delivered', @@ -1347,12 +1560,27 @@ class AgentEventService { agentName: string, instanceId: string, errorMessage: string, + deliveryId: unknown = null, ): Promise { + // D6, same reasoning as acknowledge(): failing is as terminal as acking, + // so a stale runner's failure must not retire a delivery that now belongs + // to its replacement. Match-if-present for the same migration reason — + // callers that pass nothing behave exactly as before. + const safeDeliveryId = typeof deliveryId === 'string' && deliveryId ? deliveryId : null; const result = await AgentEvent.findOneAndUpdate( - { _id: eventId, agentName: agentName.toLowerCase(), instanceId }, + { + _id: eventId, + agentName: agentName.toLowerCase(), + instanceId, + // Terminal states are terminal in both directions: without this, a + // late failure from a runner nobody is waiting for overwrites an + // event that was already acked (review on #1347). + status: { $in: ['pending', 'delivered'] }, + ...(safeDeliveryId ? { deliveryNonce: safeDeliveryId } : {}), + }, // No $inc — see acknowledge(). `attempts` is a delivery counter owned by // the claim in list(); a terminal transition must not inflate it. - { $set: { status: 'failed', error: errorMessage } }, + { $set: { status: 'failed', error: errorMessage, deliveryNonce: null } }, { new: true }, ) as EventDoc | null; diff --git a/backend/services/agentWebSocketService.ts b/backend/services/agentWebSocketService.ts index 350120e2cc..95de20a67c 100644 --- a/backend/services/agentWebSocketService.ts +++ b/backend/services/agentWebSocketService.ts @@ -55,6 +55,10 @@ interface AgentEvent { instanceId?: string; podId?: unknown; payload?: { trigger?: string }; + // ADR-026 D6, set only when the event was already claimed at enqueue time + // (native runs are born 'delivered'). Absent on the pending-queue routes — + // see the comment at the pushEvent call site in agentEventService. + deliveryId?: string; } interface AgentUserDoc { @@ -187,17 +191,54 @@ class AgentWebSocketService { socket.on('pong', () => { const data = this.connectedAgents.get(socket.agentKey); - if (data) { + // A reconnect can replace this socket for the same logical agent + // before the old connection finally times out. Do not let the old + // socket refresh the replacement's liveness timestamp. + if (data?.socket === socket) { data.lastPong = Date.now(); } }); socket.on('ack', async (payload: unknown) => { - const { eventId } = payload as { eventId?: string }; + const { eventId, deliveryId } = payload as { eventId?: string; deliveryId?: string }; if (!eventId) return; try { - await AgentEventService.acknowledge(eventId, socket.agentName, socket.instanceId); + // ADR-026 D6 at the socket boundary. This handler used to call + // acknowledge() with three arguments and emit ack:success whatever + // came back — the exact lie the two HTTP handlers grew a 400 to + // prevent, on a live surface (review on #1347). Under Phase B every + // WS ack would take the `return null` branch, the socket would + // report success, and the event would be requeued and redelivered. + // 'ws' is its own consumer key: the socket transport can be flipped + // to Phase B independently of any HTTP client (Sam's per-consumer + // steer, 2026-08-30). + if (!deliveryId && AgentEventService.isDeliveryNonceRequired('ws')) { + socket.emit('ack:error', { + eventId, + code: 'delivery_id_required', + error: 'This instance requires the deliveryId from the event payload on ack', + }); + return; + } + const acked = await AgentEventService.acknowledge( + eventId, + socket.agentName, + socket.instanceId, + null, + deliveryId || null, + 'ws', + ); + if (!acked && await AgentEventService.isSupersededDelivery( + eventId, socket.agentName, socket.instanceId, deliveryId || null, + )) { + socket.emit('ack:error', { + eventId, + code: 'stale_delivery', + error: 'This delivery was superseded by a requeue', + }); + return; + } console.log(`[agent-ws] Ack received from ${socket.agentKey} for event ${eventId}`); socket.emit('ack:success', { eventId }); } catch (err) { @@ -208,7 +249,13 @@ class AgentWebSocketService { socket.on('disconnect', (reason: unknown) => { console.log(`[agent-ws] Agent disconnected: ${socket.agentKey} (${reason})`); - this.connectedAgents.delete(socket.agentKey); + // A later connection for this agent may already have replaced this + // socket in the map. Only the socket that still owns the entry may + // remove it; otherwise an old timeout drops the live connection and + // future queue wakes have nowhere to go. + if (this.connectedAgents.get(socket.agentKey)?.socket === socket) { + this.connectedAgents.delete(socket.agentKey); + } }); socket.emit('connected', { @@ -428,15 +475,29 @@ class AgentWebSocketService { if (!this.agentNamespace) return false; const agentKey = `${event.agentName}:${event.instanceId || 'default'}`; - - this.agentNamespace.to(`agent:${agentKey}`).emit('event', event); - - if (event.podId) { - this.agentNamespace.to(`pod:${String(event.podId)}`).emit('event', event); + const target = this.connectedAgents.get(agentKey); + + if (!target) return false; + + if (event.deliveryId) { + // Native events are born claimed. Their nonce was minted with that + // claim, so this is already a delivery rather than a queue wake. Keep + // its wire shape identical to list(): every driver reads its nonce from + // payload.deliveryId, never from a transport-only top-level field. + const { deliveryId, ...wireEvent } = event; + target.socket.emit('event', { + ...wireEvent, + payload: { ...wireEvent.payload, deliveryId }, + }); + } else { + // A queued event has no delivery nonce until list() atomically claims + // it. Do not broadcast the raw pending record: a socket that processes + // that copy cannot later prove which delivery it is acknowledging. + void this.replayPendingEvents(target.socket); } console.log( - `[agent-ws] Event pushed id=${event?._id || 'n/a'} type=${event?.type || 'n/a'} ` + `[agent-ws] Event delivery requested id=${event?._id || 'n/a'} type=${event?.type || 'n/a'} ` + `agent=${agentKey} pod=${event?.podId || 'n/a'} trigger=${event?.payload?.trigger || 'n/a'}`, ); diff --git a/cli/__tests__/lib.test.mjs b/cli/__tests__/lib.test.mjs index 4069cf1158..8d513395d2 100644 --- a/cli/__tests__/lib.test.mjs +++ b/cli/__tests__/lib.test.mjs @@ -461,8 +461,9 @@ describe('poller.js', () => { expect(received[1]._id).toBe('e2'); }); - test('posts to /api/agents/runtime/events/acknowledge after each event', async () => { + test('binds an ack to the delivery id returned by the poll', async () => { const events = [makeEvent('ack-1')]; + events[0].payload.deliveryId = 'delivery-abc'; const mockGet = jest.fn().mockResolvedValue({ events }); const mockPost = jest.fn().mockResolvedValue({}); createClient.mockReturnValue({ get: mockGet, post: mockPost }); @@ -480,7 +481,10 @@ describe('poller.js', () => { expect(mockPost).toHaveBeenCalledWith( '/api/agents/runtime/events/ack-1/ack', - expect.objectContaining({ result: { outcome: 'acknowledged' } }), + expect.objectContaining({ + result: { outcome: 'acknowledged' }, + deliveryId: 'delivery-abc', + }), ); }); diff --git a/cli/__tests__/run-loop.test.mjs b/cli/__tests__/run-loop.test.mjs index 6716a99ef3..d456fe8f04 100644 --- a/cli/__tests__/run-loop.test.mjs +++ b/cli/__tests__/run-loop.test.mjs @@ -81,7 +81,7 @@ describe('performRun', () => { }); test('event with content → adapter.spawn → message posted → event acked', async () => { - const events = [makeEvent()]; + const events = [makeEvent({ payload: { content: 'hello from tester', deliveryId: 'delivery-abc' } })]; const mockGet = jest.fn().mockResolvedValue({ events }); const mockPost = jest.fn().mockResolvedValue({}); createClient.mockReturnValue({ get: mockGet, post: mockPost }); @@ -121,7 +121,7 @@ describe('performRun', () => { ); expect(mockPost).toHaveBeenCalledWith( '/api/agents/runtime/events/evt-1/ack', - { result: { outcome: 'posted' } }, + { result: { outcome: 'posted' }, deliveryId: 'delivery-abc' }, ); }); diff --git a/cli/package-lock.json b/cli/package-lock.json index c59d81d234..46e1cf52e3 100644 --- a/cli/package-lock.json +++ b/cli/package-lock.json @@ -1,12 +1,12 @@ { "name": "@commonlyai/cli", - "version": "0.1.25", + "version": "0.1.26", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@commonlyai/cli", - "version": "0.1.25", + "version": "0.1.26", "license": "Apache-2.0", "dependencies": { "commander": "^12.0.0" diff --git a/cli/package.json b/cli/package.json index 54353a8571..a783161336 100644 --- a/cli/package.json +++ b/cli/package.json @@ -1,6 +1,6 @@ { "name": "@commonlyai/cli", - "version": "0.1.25", + "version": "0.1.26", "license": "Apache-2.0", "description": "The Commonly CLI \u2014 connect agents, manage pods, iterate fast", "type": "module", diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index 93c6b9e4cc..dd87f18054 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -1344,7 +1344,11 @@ export const performRun = ({ recordHandledEvent(agentName, event._id); } try { - await client.post(`/api/agents/runtime/events/${event._id}/ack`, { result }); + const deliveryId = event.payload?.deliveryId; + await client.post(`/api/agents/runtime/events/${event._id}/ack`, { + result, + ...(typeof deliveryId === 'string' && deliveryId ? { deliveryId } : {}), + }); } catch (ackErr) { onError?.(new Error(`Ack failed for ${event._id}: ${ackErr.message}`)); } diff --git a/cli/src/lib/poller.js b/cli/src/lib/poller.js index 1bb703c342..257f14142b 100644 --- a/cli/src/lib/poller.js +++ b/cli/src/lib/poller.js @@ -47,8 +47,10 @@ export const startPoller = ({ // Acknowledge the event try { + const deliveryId = event.payload?.deliveryId; await client.post(`/api/agents/runtime/events/${event._id}/ack`, { result, + ...(typeof deliveryId === 'string' && deliveryId ? { deliveryId } : {}), }); } catch (ackErr) { // Non-fatal — event will be retried diff --git a/docs-site/agents/connect.mdx b/docs-site/agents/connect.mdx index 2f4f81f1dc..55ae2273be 100644 --- a/docs-site/agents/connect.mdx +++ b/docs-site/agents/connect.mdx @@ -87,8 +87,9 @@ curl -X POST https://api.commonly.me/api/registry/pods/:podId/agents/:name/runti curl -H "Authorization: Bearer cm_agent_..." \ "https://api.commonly.me/api/agents/runtime/events?timeout=30" -# Acknowledge an event -curl -X POST -H "Authorization: Bearer cm_agent_..." \ +# Acknowledge an event; echo payload.deliveryId when the polled event has one +curl -X POST -H "Authorization: Bearer cm_agent_..." -H "Content-Type: application/json" \ + -d '{"deliveryId":""}' \ "https://api.commonly.me/api/agents/runtime/events/:id/ack" # Post a reply diff --git a/docs-site/agents/events.mdx b/docs-site/agents/events.mdx index ce4389f7bf..8e02669ce5 100644 --- a/docs-site/agents/events.mdx +++ b/docs-site/agents/events.mdx @@ -108,7 +108,13 @@ Events must be acknowledged or they will be redelivered: ```bash curl -X POST \ -H "Authorization: Bearer cm_agent_..." \ + -H "Content-Type: application/json" \ + -d '{"deliveryId":""}' \ "https://api.commonly.me/api/agents/runtime/events/:id/ack" ``` +When a polled event includes `payload.deliveryId`, echo that exact value on +the acknowledgement. It binds the ack to this delivery generation; never +invent one for an older event that has no value. + `delivered: true` on an event means the runtime acknowledged receipt — it does not guarantee a chat message was posted. diff --git a/docs/agents/WEBHOOK_SDK.md b/docs/agents/WEBHOOK_SDK.md index 4400c7511c..002b9f7409 100644 --- a/docs/agents/WEBHOOK_SDK.md +++ b/docs/agents/WEBHOOK_SDK.md @@ -101,7 +101,7 @@ There is no `commonly agent detach` for webhook-SDK installs yet — the CLI's ` | Verb | Method | Endpoint | |------|--------|----------| | Poll events | `bot.poll_events(limit=10)` | `GET /api/agents/runtime/events` | -| Ack event | `bot.ack(event_id, outcome="posted")` | `POST /api/agents/runtime/events/:id/ack` | +| Ack event | `bot.ack(event_id, delivery_id=event["payload"].get("deliveryId"), outcome="posted")` | `POST /api/agents/runtime/events/:id/ack` | | Post message | `bot.post_message(pod_id, content)` | `POST /api/agents/runtime/pods/:podId/messages` | | Get memory | `bot.get_memory()` | `GET /api/agents/runtime/memory` | | Sync memory | `bot.sync_memory(sections, mode="patch")` | `POST /api/agents/runtime/memory/sync` | diff --git a/examples/sdk/python/commonly.py b/examples/sdk/python/commonly.py index b028978a64..b2e9044b2c 100644 --- a/examples/sdk/python/commonly.py +++ b/examples/sdk/python/commonly.py @@ -21,7 +21,7 @@ if reply and evt.get("podId"): bot.post_message(evt["podId"], reply) finally: - bot.ack(evt["_id"]) + bot.ack(evt["_id"], delivery_id=evt.get("payload", {}).get("deliveryId")) time.sleep(5) """ @@ -97,15 +97,18 @@ def poll_events(self, *, limit: int = 10) -> list: body = self._request("GET", self._path("/api/agents/runtime/events", limit=int(limit))) return body.get("events", []) if isinstance(body, dict) else [] - def ack(self, event_id: str, *, outcome: str = "acknowledged", - content: Optional[str] = None) -> None: - """CAP verb 2 — acknowledge an event. Skip on adapter errors so the + def ack(self, event_id: str, *, delivery_id: Optional[str] = None, + outcome: str = "acknowledged", content: Optional[str] = None) -> None: + """CAP verb 2 — acknowledge an event. Forward the delivery_id from + event.payload.deliveryId when present; skip on adapter errors so the kernel re-delivers (ADR-005 §Spawning semantics).""" result = {"outcome": outcome} if content is not None: result["content"] = content - self._request("POST", f"/api/agents/runtime/events/{event_id}/ack", - {"result": result}) + body: dict = {"result": result} + if delivery_id: + body["deliveryId"] = delivery_id + self._request("POST", f"/api/agents/runtime/events/{event_id}/ack", body) def post_message(self, pod_id: str, content: str, *, reply_to_message_id: Optional[str] = None, @@ -184,7 +187,11 @@ def run(self, on_event: Callable[[dict], Optional[str]], *, print(f"[commonly] post failed ({exc.status}): {exc}", flush=True) continue try: - self.ack(evt["_id"], outcome="posted" if reply else "no_action") + self.ack( + evt["_id"], + delivery_id=evt.get("payload", {}).get("deliveryId"), + outcome="posted" if reply else "no_action", + ) except CommonlyError as exc: print(f"[commonly] ack failed ({exc.status}): {exc}", flush=True) time.sleep(interval_s) diff --git a/external/commonly-agent-services/clawdbot-bridge/index.js b/external/commonly-agent-services/clawdbot-bridge/index.js index 2cb3401b28..6c6fa189a8 100644 --- a/external/commonly-agent-services/clawdbot-bridge/index.js +++ b/external/commonly-agent-services/clawdbot-bridge/index.js @@ -11,6 +11,8 @@ * agent:messages:read, agent:messages:write */ +const { ackBodyForEvent } = require('../shared/delivery-nonce'); + const baseUrl = process.env.COMMONLY_BASE_URL || 'http://backend:5000'; const userToken = process.env.COMMONLY_USER_TOKEN; const agentToken = process.env.COMMONLY_AGENT_TOKEN; @@ -76,11 +78,14 @@ const fetchRuntimeEvents = async () => { return data.events || []; }; -const ackRuntimeEvent = async (eventId, result = null) => { - const res = await fetch(`${baseUrl}/api/agents/runtime/events/${eventId}/ack`, { +const ackRuntimeEvent = async (event, result = null) => { + const res = await fetch(`${baseUrl}/api/agents/runtime/events/${event._id}/ack`, { method: 'POST', headers: runtimeHeaders, - body: result ? JSON.stringify({ result }) : undefined, + body: JSON.stringify({ + ...ackBodyForEvent(event), + ...(result ? { result } : {}), + }), }); if (!res.ok) { throw new Error(`Failed to ack event: ${res.status}`); @@ -216,16 +221,17 @@ const postThreadComment = async (threadId, content, podId = null) => { /** * Acknowledge event via bot API */ -const ackEvent = async (eventId, result = null) => { +const ackEvent = async (event, result = null) => { if (runtimeHeaders) { - return ackRuntimeEvent(eventId, result); + return ackRuntimeEvent(event, result); } - const res = await fetch(`${baseUrl}/api/agents/runtime/bot/events/${eventId}/ack`, { + const res = await fetch(`${baseUrl}/api/agents/runtime/bot/events/${event._id}/ack`, { method: 'POST', headers: botHeaders, body: JSON.stringify({ agentName: agentType, instanceId, + ...ackBodyForEvent(event), ...(result ? { result } : {}), }), }); @@ -512,10 +518,10 @@ const handleMentionEvent = async (event) => { const { content, username, messageId } = event.payload || {}; if (!content) { - return ackEvent(event._id, { outcome: 'no_action', reason: 'empty_mention_content' }); + return ackEvent(event, { outcome: 'no_action', reason: 'empty_mention_content' }); } if (processedEvents.has(event._id)) { - return ackEvent(event._id, { outcome: 'skipped', reason: 'duplicate_event_id' }); + return ackEvent(event, { outcome: 'skipped', reason: 'duplicate_event_id' }); } processedEvents.add(event._id); @@ -541,7 +547,7 @@ const handleMentionEvent = async (event) => { ); if (!response) { - return ackEvent(event._id, { outcome: 'no_action', reason: 'empty_model_response' }); + return ackEvent(event, { outcome: 'no_action', reason: 'empty_model_response' }); } // Post response to pod @@ -559,7 +565,7 @@ const handleMentionEvent = async (event) => { }); console.log(`Responded to @${username} with context-aware message`); - return ackEvent(event._id, { + return ackEvent(event, { outcome: 'posted', reason: 'mention_response_posted', messageId: posted?.id || posted?._id || null, @@ -577,7 +583,7 @@ const handleMentionEvent = async (event) => { eventId: event._id, fallback: true, }); - return ackEvent(event._id, { + return ackEvent(event, { outcome: 'posted', reason: 'mention_fallback_posted', messageId: posted?.id || posted?._id || null, @@ -586,7 +592,7 @@ const handleMentionEvent = async (event) => { } catch (fallbackErr) { console.error(`Fallback also failed: ${fallbackErr.message}`); } - return ackEvent(event._id, { outcome: 'error', reason: err.message || 'mention_handler_failed' }); + return ackEvent(event, { outcome: 'error', reason: err.message || 'mention_handler_failed' }); } }; @@ -600,10 +606,10 @@ const handleThreadMentionEvent = async (event) => { const threadId = thread.postId || payload.threadId; if (!threadId || !content) { - return ackEvent(event._id, { outcome: 'no_action', reason: 'thread_context_missing' }); + return ackEvent(event, { outcome: 'no_action', reason: 'thread_context_missing' }); } if (processedEvents.has(event._id)) { - return ackEvent(event._id, { outcome: 'skipped', reason: 'duplicate_event_id' }); + return ackEvent(event, { outcome: 'skipped', reason: 'duplicate_event_id' }); } processedEvents.add(event._id); @@ -632,19 +638,19 @@ const handleThreadMentionEvent = async (event) => { ); if (!response) { - return ackEvent(event._id, { outcome: 'no_action', reason: 'empty_model_response' }); + return ackEvent(event, { outcome: 'no_action', reason: 'empty_model_response' }); } await postThreadComment(threadId, response, event.podId); console.log(`Responded to thread mention from @${username}`); - return ackEvent(event._id, { + return ackEvent(event, { outcome: 'posted', reason: 'thread_response_posted', messageId: threadId, }); } catch (err) { console.error(`Failed to handle thread mention: ${err.message}`); - return ackEvent(event._id, { outcome: 'error', reason: err.message || 'thread_handler_failed' }); + return ackEvent(event, { outcome: 'error', reason: err.message || 'thread_handler_failed' }); } }; @@ -654,10 +660,10 @@ const handleThreadMentionEvent = async (event) => { const handleSummaryEvent = async (event) => { const summaryContent = event.payload?.summary?.content || event.payload?.summary; if (!summaryContent) { - return ackEvent(event._id, { outcome: 'no_action', reason: 'summary_missing_content' }); + return ackEvent(event, { outcome: 'no_action', reason: 'summary_missing_content' }); } if (processedEvents.has(event._id)) { - return ackEvent(event._id, { outcome: 'skipped', reason: 'duplicate_event_id' }); + return ackEvent(event, { outcome: 'skipped', reason: 'duplicate_event_id' }); } processedEvents.add(event._id); @@ -670,16 +676,16 @@ const handleSummaryEvent = async (event) => { source: agentType, eventId: event._id, }); - return ackEvent(event._id, { + return ackEvent(event, { outcome: 'posted', reason: 'summary_response_posted', messageId: posted?.id || posted?._id || null, }); } - return ackEvent(event._id, { outcome: 'no_action', reason: 'summary_model_empty' }); + return ackEvent(event, { outcome: 'no_action', reason: 'summary_model_empty' }); } catch (err) { console.error(`Failed to handle summary: ${err.message}`); - return ackEvent(event._id, { outcome: 'error', reason: err.message || 'summary_handler_failed' }); + return ackEvent(event, { outcome: 'error', reason: err.message || 'summary_handler_failed' }); } }; @@ -703,7 +709,7 @@ const handleEvent = async (event) => { // Unknown event type - just ack it console.log(`Unknown event type: ${event.type}`); - return ackEvent(event._id, { outcome: 'skipped', reason: `unsupported_event_type:${event.type}` }); + return ackEvent(event, { outcome: 'skipped', reason: `unsupported_event_type:${event.type}` }); }; // ============================================================================ diff --git a/external/commonly-agent-services/commonly-bot/index.js b/external/commonly-agent-services/commonly-bot/index.js index 980e1f47f5..1a33af50f3 100644 --- a/external/commonly-agent-services/commonly-bot/index.js +++ b/external/commonly-agent-services/commonly-bot/index.js @@ -1,5 +1,6 @@ const fs = require('fs'); const LiteLLMClient = require('../shared/litellm-client'); +const { ackBodyForEvent } = require('../shared/delivery-nonce'); const baseUrl = process.env.COMMONLY_BASE_URL || 'http://localhost:5000'; const token = process.env.COMMONLY_AGENT_TOKEN; const userToken = process.env.COMMONLY_USER_TOKEN; @@ -599,10 +600,13 @@ const persistSummary = async (runtimeToken, podId, { return res.json(); }; -const ackEvent = async (runtimeToken, eventId) => { +const ackEvent = async (runtimeToken, event) => { + const eventId = event?._id; + if (!eventId) throw new Error('Cannot acknowledge an event without an id'); const res = await fetch(`${baseUrl}/api/agents/runtime/events/${eventId}/ack`, { method: 'POST', headers: buildHeaders(runtimeToken), + body: JSON.stringify(ackBodyForEvent(event)), }); if (!res.ok) { throw new Error(`Failed to ack event: ${res.status}`); @@ -630,7 +634,7 @@ const handleEvent = async (account, event) => { }); if (!digest) { - await ackEvent(runtimeToken, event._id); + await ackEvent(runtimeToken, event); return; } @@ -729,7 +733,7 @@ const handleEvent = async (account, event) => { } } - await ackEvent(runtimeToken, event._id); + await ackEvent(runtimeToken, event); return; } @@ -746,7 +750,7 @@ const handleEvent = async (account, event) => { podName: context?.pod?.name || 'this pod', }); if (!synthetic?.summary) { - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); } if (silentDelivery) { @@ -758,7 +762,7 @@ const handleEvent = async (account, event) => { messageCount: synthetic.messageCount || 0, eventId: event._id?.toString?.() || event._id, }); - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); } const content = `[BOT_MESSAGE]${JSON.stringify(synthetic)}`; @@ -768,20 +772,20 @@ const handleEvent = async (account, event) => { summaryType: 'chats', messageCount: synthetic.messageCount, }); - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); } if (!event?.payload?.summary) { - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); } if (silentDelivery) { - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); } const content = formatIntegrationSummary(event.payload.summary, event.payload.source); if (!content) { - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); } await postMessage(runtimeToken, event.podId, content, { @@ -791,7 +795,7 @@ const handleEvent = async (account, event) => { messageCount: event.payload?.summary?.messageCount || 0, }); - return ackEvent(runtimeToken, event._id); + return ackEvent(runtimeToken, event); }; const pollAccount = async (account) => { diff --git a/external/commonly-agent-services/shared/bridge-base.js b/external/commonly-agent-services/shared/bridge-base.js index 0c04cbbaab..2637e693c7 100644 --- a/external/commonly-agent-services/shared/bridge-base.js +++ b/external/commonly-agent-services/shared/bridge-base.js @@ -70,10 +70,13 @@ class BridgeBase { /** * Acknowledge an event */ - async ackEvent(eventId) { + async ackEvent(eventId, deliveryId) { const res = await fetch(`${this.baseUrl}/api/agents/runtime/events/${eventId}/ack`, { method: 'POST', headers: this.headers, + body: JSON.stringify( + typeof deliveryId === 'string' && deliveryId ? { deliveryId } : {}, + ), }); if (!res.ok) { throw new Error(`Failed to ack event: ${res.status}`); @@ -200,7 +203,7 @@ class BridgeBase { const events = await this.fetchEvents(); for (const event of events) { if (this.wasProcessed(event._id)) { - await this.ackEvent(event._id); + await this.ackEvent(event._id, event.payload?.deliveryId); continue; } try { @@ -209,7 +212,7 @@ class BridgeBase { } catch (err) { console.error(`Error handling event ${event._id}:`, err.message); } - await this.ackEvent(event._id); + await this.ackEvent(event._id, event.payload?.deliveryId); } } catch (error) { console.error('Poll failed:', error.message); diff --git a/external/commonly-agent-services/shared/delivery-nonce.js b/external/commonly-agent-services/shared/delivery-nonce.js new file mode 100644 index 0000000000..41e3dd7dd5 --- /dev/null +++ b/external/commonly-agent-services/shared/delivery-nonce.js @@ -0,0 +1,11 @@ +/** + * Keep D6's delivery generation with the event that was claimed. Consumers + * must not manufacture a nonce: an absent value preserves the Phase-A + * compatibility path until every deployed client has upgraded. + */ +const ackBodyForEvent = (event) => { + const deliveryId = event?.payload?.deliveryId; + return typeof deliveryId === 'string' && deliveryId ? { deliveryId } : {}; +}; + +module.exports = { ackBodyForEvent }; diff --git a/external/commonly-agent-services/shared/delivery-nonce.test.js b/external/commonly-agent-services/shared/delivery-nonce.test.js new file mode 100644 index 0000000000..240e544066 --- /dev/null +++ b/external/commonly-agent-services/shared/delivery-nonce.test.js @@ -0,0 +1,50 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { ackBodyForEvent } = require('./delivery-nonce'); +const BridgeBase = require('./bridge-base'); + +test('uses the delivery nonce from the claimed event payload', () => { + assert.deepEqual(ackBodyForEvent({ payload: { deliveryId: 'claimed-child' } }), { + deliveryId: 'claimed-child', + }); +}); + +test('does not invent a nonce for a pre-D6 event', () => { + assert.deepEqual(ackBodyForEvent({ payload: {} }), {}); + assert.deepEqual(ackBodyForEvent(undefined), {}); +}); + +test('the generic bridge echoes the claimed nonce on its ack wire call', async () => { + const previousFetch = global.fetch; + let request; + global.fetch = async (url, options) => { + request = { url, options }; + return { ok: true }; + }; + + try { + const bridge = new BridgeBase({ baseUrl: 'https://api.example', agentToken: 'cm_agent_test' }); + await bridge.ackEvent('evt-1', 'claimed-child'); + assert.equal(request.url, 'https://api.example/api/agents/runtime/events/evt-1/ack'); + assert.deepEqual(JSON.parse(request.options.body), { deliveryId: 'claimed-child' }); + } finally { + global.fetch = previousFetch; + } +}); + +test('the generic bridge keeps the Phase-A empty body for a legacy event', async () => { + const previousFetch = global.fetch; + let request; + global.fetch = async (url, options) => { + request = { url, options }; + return { ok: true }; + }; + + try { + const bridge = new BridgeBase({ baseUrl: 'https://api.example', agentToken: 'cm_agent_test' }); + await bridge.ackEvent('legacy-event'); + assert.deepEqual(JSON.parse(request.options.body), {}); + } finally { + global.fetch = previousFetch; + } +}); diff --git a/packages/commonly-mcp/README.md b/packages/commonly-mcp/README.md index 1d5b2c25b6..297ce9d190 100644 --- a/packages/commonly-mcp/README.md +++ b/packages/commonly-mcp/README.md @@ -191,7 +191,7 @@ A typical CAP loop, as the agent would express it through MCP tool calls: > commonly_poll_events() { "events": [ { "id": "evt_42", "type": "mention.received", - "payload": { "podId": "pod_abc", "messageId": "msg_99", "text": "@bot ping?" } } + "payload": { "podId": "pod_abc", "messageId": "msg_99", "text": "@bot ping?", "deliveryId": "claimed-child" } } ] } > commonly_post_message_cap({ "podId": "pod_abc", "content": "pong", @@ -199,7 +199,7 @@ A typical CAP loop, as the agent would express it through MCP tool calls: { "messageId": "msg_100", "podId": "pod_abc", "createdAt": "2026-04-16T10:00:00.000Z" } -> commonly_ack_event({ "eventId": "evt_42" }) +> commonly_ack_event({ "eventId": "evt_42", "deliveryId": "claimed-child" }) { "ok": true } ``` diff --git a/packages/commonly-mcp/src/__tests__/cap-tools.test.ts b/packages/commonly-mcp/src/__tests__/cap-tools.test.ts index f5a836b63b..6f36352c57 100644 --- a/packages/commonly-mcp/src/__tests__/cap-tools.test.ts +++ b/packages/commonly-mcp/src/__tests__/cap-tools.test.ts @@ -83,7 +83,7 @@ describe("commonly_poll_events", () => { }); describe("commonly_ack_event", () => { - it("calls ackEvent with the eventId", async () => { + it("calls ackEvent with the claimed delivery nonce", async () => { const client = { ackEvent: vi.fn().mockResolvedValue({ success: true }), } as unknown as CommonlyClient; @@ -91,11 +91,11 @@ describe("commonly_ack_event", () => { const result = await handleToolCall( client, "commonly_ack_event", - { eventId: "evt_123" }, + { eventId: "evt_123", deliveryId: "claimed-child" }, baseConfig() ); - expect(client.ackEvent).toHaveBeenCalledWith("evt_123"); + expect(client.ackEvent).toHaveBeenCalledWith("evt_123", "claimed-child"); expect(result).toEqual({ ok: true }); }); diff --git a/packages/commonly-mcp/src/__tests__/client-cap.test.ts b/packages/commonly-mcp/src/__tests__/client-cap.test.ts index 0e45a114dd..5080b38737 100644 --- a/packages/commonly-mcp/src/__tests__/client-cap.test.ts +++ b/packages/commonly-mcp/src/__tests__/client-cap.test.ts @@ -127,7 +127,7 @@ describe("CommonlyClient CAP methods — URL construction", () => { expect(call.params).toEqual({}); }); - it("ackEvent URL-encodes the event id", async () => { + it("ackEvent URL-encodes the event id and forwards its delivery nonce", async () => { const client = new CommonlyClient({ apiUrl: "https://api.commonly.app", agentToken: "cm_agent_x", @@ -135,12 +135,32 @@ describe("CommonlyClient CAP methods — URL construction", () => { const agentInst = createdInstances[0]; agentInst.request.mockResolvedValueOnce({ data: { success: true } }); - await client.ackEvent("evt/with slash"); + await client.ackEvent("evt/with slash", "claimed-child"); expect(agentInst.request).toHaveBeenCalledWith( expect.objectContaining({ method: "post", url: "/api/agents/runtime/events/evt%2Fwith%20slash/ack", + data: { deliveryId: "claimed-child" }, + }) + ); + }); + + it("keeps Phase-A compatibility for an event delivered before nonces", async () => { + const client = new CommonlyClient({ + apiUrl: "https://api.commonly.app", + agentToken: "cm_agent_x", + }); + const agentInst = createdInstances[0]; + agentInst.request.mockResolvedValueOnce({ data: { success: true } }); + + await client.ackEvent("legacy-event"); + + expect(agentInst.request).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: "/api/agents/runtime/events/legacy-event/ack", + data: {}, }) ); }); diff --git a/packages/commonly-mcp/src/client.ts b/packages/commonly-mcp/src/client.ts index a5e61f0bca..8c0c6319d0 100644 --- a/packages/commonly-mcp/src/client.ts +++ b/packages/commonly-mcp/src/client.ts @@ -52,13 +52,14 @@ export class MCPClientError extends Error { /** * Single CAP event as returned by GET /api/agents/runtime/events. - * Shape per ADR-004 §Event model. `payload` is type-specific and opaque - * to the client. + * Shape per ADR-004 §Event model. `payload` is type-specific, except that + * a claimed delivery carries its generation at `payload.deliveryId`; send it + * unchanged to ackEvent so an old child cannot acknowledge a requeued child. */ export interface CAPEvent { id: string; type: string; - payload?: Record; + payload?: { deliveryId?: string; [key: string]: unknown }; attempts?: number; createdAt?: string; [key: string]: unknown; @@ -543,14 +544,14 @@ export class CommonlyClient { * Drivers MUST call this after successful handling, or the event will * re-deliver on the next poll (ADR-004 §Event model). */ - async ackEvent(eventId: string): Promise { + async ackEvent(eventId: string, deliveryId?: string): Promise { if (!eventId) { throw new MCPClientError("ackEvent requires a non-empty eventId"); } return this._capRequest( "post", `/events/${encodeURIComponent(eventId)}/ack`, - {} + typeof deliveryId === "string" && deliveryId ? { deliveryId } : {} ); } diff --git a/packages/commonly-mcp/src/tools/cap-ack.ts b/packages/commonly-mcp/src/tools/cap-ack.ts index 313a1e584c..7f6ee6ce87 100644 --- a/packages/commonly-mcp/src/tools/cap-ack.ts +++ b/packages/commonly-mcp/src/tools/cap-ack.ts @@ -21,6 +21,10 @@ export const definition = { type: "string", description: "The event id returned by commonly_poll_events.", }, + deliveryId: { + type: "string", + description: "The payload.deliveryId returned with the claimed event, when present.", + }, }, required: ["eventId"], }, @@ -28,13 +32,14 @@ export const definition = { export interface CapAckArgs { eventId: string; + deliveryId?: string; } export async function handler( client: CommonlyClient, args: CapAckArgs ): Promise<{ ok: true }> { - await client.ackEvent(args.eventId); + await client.ackEvent(args.eventId, args.deliveryId); // Backend returns { success: true } on the wire; we standardize to { ok: true } // in the tool surface so downstream agents can rely on a single shape. return { ok: true }; diff --git a/packages/commonly-mcp/src/tools/index.ts b/packages/commonly-mcp/src/tools/index.ts index 71df1027fe..53d2175999 100644 --- a/packages/commonly-mcp/src/tools/index.ts +++ b/packages/commonly-mcp/src/tools/index.ts @@ -391,7 +391,10 @@ export async function handleToolCall( case CapAck.definition.name: { const eventId = args.eventId as string | undefined; if (!eventId) throw new Error("eventId is required"); - return CapAck.handler(client, { eventId }); + return CapAck.handler(client, { + eventId, + deliveryId: args.deliveryId as string | undefined, + }); } case CapPost.definition.name: {