fix(agent-to-agent): route A2A replies back to originating session (#2267)
Squash merge of PR #2267 by ddaniels. When an agent group has more than one active session, A2A replies landed in the newest session via findSessionByAgentGroup's ORDER BY created_at DESC. The session that asked the question never saw the answer. Adds origin-aware return-path routing with three layers: 1. Direct return-path: if the reply has in_reply_to, look up the triggering inbound row's source_session_id and route there. 2. Peer-affinity fallback: find the most recent A2A inbound from this peer and use its source_session_id. 3. Legacy fallback: newest active session (pre-migration compat). Container-side: MCP send_message/send_file now thread the current batch's in_reply_to through to outbound rows via current-batch.ts. Also flips our A2A bug-documenting test (#2332) from asserting the broken behavior to asserting the fixed behavior. Co-Authored-By: Doug Daniels <ddaniels888@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,20 +1,53 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import Database from 'better-sqlite3';
|
||||
import fs from 'fs';
|
||||
import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
|
||||
|
||||
import { isSafeAttachmentName } from './agent-route.js';
|
||||
import { isSafeAttachmentName, routeAgentMessage } from './agent-route.js';
|
||||
import { createDestination } from './db/agent-destinations.js';
|
||||
import { initTestDb, closeDb, runMigrations, createAgentGroup } from '../../db/index.js';
|
||||
import { createSession } from '../../db/sessions.js';
|
||||
import { initSessionFolder, inboundDbPath } from '../../session-manager.js';
|
||||
import type { Session } from '../../types.js';
|
||||
|
||||
vi.mock('../../container-runner.js', () => ({
|
||||
wakeContainer: vi.fn().mockResolvedValue(undefined),
|
||||
isContainerRunning: vi.fn().mockReturnValue(false),
|
||||
getActiveContainerCount: vi.fn().mockReturnValue(0),
|
||||
killContainer: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('../../config.js', async () => {
|
||||
const actual = await vi.importActual('../../config.js');
|
||||
return { ...actual, DATA_DIR: '/tmp/nanoclaw-test-a2a-route' };
|
||||
});
|
||||
|
||||
const TEST_DIR = '/tmp/nanoclaw-test-a2a-route';
|
||||
|
||||
function now(): string {
|
||||
return new Date().toISOString();
|
||||
}
|
||||
|
||||
function readInbound(agentGroupId: string, sessionId: string) {
|
||||
const db = new Database(inboundDbPath(agentGroupId, sessionId), { readonly: true });
|
||||
const rows = db
|
||||
.prepare('SELECT id, platform_id, channel_type, content, source_session_id FROM messages_in ORDER BY seq')
|
||||
.all() as Array<{
|
||||
id: string;
|
||||
platform_id: string | null;
|
||||
channel_type: string | null;
|
||||
content: string;
|
||||
source_session_id: string | null;
|
||||
}>;
|
||||
db.close();
|
||||
return rows;
|
||||
}
|
||||
|
||||
/**
|
||||
* `forwardAttachedFiles` has a filesystem side that's awkward to unit-test
|
||||
* without mocking DATA_DIR. The guarantee worth pinning is that the
|
||||
* filename validator rejects everything that could escape the inbox dir —
|
||||
* `forwardAttachedFiles` runs this guard before any I/O, so traversal is
|
||||
* impossible as long as this matrix holds.
|
||||
*/
|
||||
describe('isSafeAttachmentName', () => {
|
||||
it('accepts plain filenames', () => {
|
||||
expect(isSafeAttachmentName('baby-duck.png')).toBe(true);
|
||||
expect(isSafeAttachmentName('file with spaces.pdf')).toBe(true);
|
||||
expect(isSafeAttachmentName('report.v2.docx')).toBe(true);
|
||||
expect(isSafeAttachmentName('.hidden')).toBe(true); // leading dot is fine, just not `.` / `..`
|
||||
expect(isSafeAttachmentName('.hidden')).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects empty / sentinel values', () => {
|
||||
@@ -44,3 +77,200 @@ describe('isSafeAttachmentName', () => {
|
||||
expect(isSafeAttachmentName(undefined as unknown as string)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Return-path routing: when an a2a reply targets an agent group with multiple
|
||||
* sessions, it must land in the *originating* session — not the newest one.
|
||||
*
|
||||
* Setup: agent A has two active sessions S1 (older) + S2 (newer).
|
||||
* Agent B is the peer A talks to. Bidirectional destinations wired.
|
||||
*/
|
||||
describe('routeAgentMessage return-path', () => {
|
||||
const A = 'ag-A';
|
||||
const B = 'ag-B';
|
||||
let S1: Session;
|
||||
let S2: Session;
|
||||
let SB: Session;
|
||||
|
||||
beforeEach(() => {
|
||||
if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true });
|
||||
fs.mkdirSync(TEST_DIR, { recursive: true });
|
||||
|
||||
const db = initTestDb();
|
||||
runMigrations(db);
|
||||
|
||||
createAgentGroup({ id: A, name: 'A', folder: 'a', agent_provider: null, created_at: now() });
|
||||
createAgentGroup({ id: B, name: 'B', folder: 'b', agent_provider: null, created_at: now() });
|
||||
|
||||
// S1 (older), S2 (newer) — both active sessions on A.
|
||||
S1 = {
|
||||
id: 'sess-A-old',
|
||||
agent_group_id: A,
|
||||
messaging_group_id: null,
|
||||
thread_id: null,
|
||||
agent_provider: null,
|
||||
status: 'active',
|
||||
container_status: 'stopped',
|
||||
last_active: null,
|
||||
created_at: '2026-01-01T00:00:00.000Z',
|
||||
};
|
||||
S2 = {
|
||||
id: 'sess-A-new',
|
||||
agent_group_id: A,
|
||||
messaging_group_id: null,
|
||||
thread_id: null,
|
||||
agent_provider: null,
|
||||
status: 'active',
|
||||
container_status: 'stopped',
|
||||
last_active: null,
|
||||
created_at: '2026-02-01T00:00:00.000Z',
|
||||
};
|
||||
SB = {
|
||||
id: 'sess-B',
|
||||
agent_group_id: B,
|
||||
messaging_group_id: null,
|
||||
thread_id: null,
|
||||
agent_provider: null,
|
||||
status: 'active',
|
||||
container_status: 'stopped',
|
||||
last_active: null,
|
||||
created_at: '2026-01-15T00:00:00.000Z',
|
||||
};
|
||||
createSession(S1);
|
||||
createSession(S2);
|
||||
createSession(SB);
|
||||
initSessionFolder(A, S1.id);
|
||||
initSessionFolder(A, S2.id);
|
||||
initSessionFolder(B, SB.id);
|
||||
|
||||
createDestination({
|
||||
agent_group_id: A,
|
||||
local_name: 'b',
|
||||
target_type: 'agent',
|
||||
target_id: B,
|
||||
created_at: now(),
|
||||
});
|
||||
createDestination({
|
||||
agent_group_id: B,
|
||||
local_name: 'a',
|
||||
target_type: 'agent',
|
||||
target_id: A,
|
||||
created_at: now(),
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
closeDb();
|
||||
if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true });
|
||||
});
|
||||
|
||||
it('forward direction: stamps source_session_id on the target inbound row', async () => {
|
||||
// A.S1 emits an outbound a2a to B.
|
||||
await routeAgentMessage(
|
||||
{
|
||||
id: 'msg-from-A-S1',
|
||||
platform_id: B,
|
||||
content: JSON.stringify({ text: 'hello B' }),
|
||||
in_reply_to: null,
|
||||
},
|
||||
S1,
|
||||
);
|
||||
|
||||
const bRows = readInbound(B, SB.id);
|
||||
expect(bRows).toHaveLength(1);
|
||||
expect(bRows[0].platform_id).toBe(A);
|
||||
expect(bRows[0].source_session_id).toBe(S1.id); // <- the return address
|
||||
});
|
||||
|
||||
it('reply direction: routes back to the originating session, not the newest', async () => {
|
||||
// A.S1 sends to B.
|
||||
await routeAgentMessage(
|
||||
{
|
||||
id: 'msg-from-A-S1',
|
||||
platform_id: B,
|
||||
content: JSON.stringify({ text: 'ping' }),
|
||||
in_reply_to: null,
|
||||
},
|
||||
S1,
|
||||
);
|
||||
|
||||
// Capture the synthetic id the host stamped on B's inbound — that's what
|
||||
// B's container would reference as `in_reply_to` when replying.
|
||||
const bRows = readInbound(B, SB.id);
|
||||
const yId = bRows[0].id;
|
||||
|
||||
// B replies to that message.
|
||||
await routeAgentMessage(
|
||||
{
|
||||
id: 'msg-from-B',
|
||||
platform_id: A,
|
||||
content: JSON.stringify({ text: 'pong' }),
|
||||
in_reply_to: yId,
|
||||
},
|
||||
SB,
|
||||
);
|
||||
|
||||
const s1Rows = readInbound(A, S1.id);
|
||||
const s2Rows = readInbound(A, S2.id);
|
||||
|
||||
// The reply lands in S1 (originator) even though S2 is newer.
|
||||
expect(s1Rows).toHaveLength(1);
|
||||
expect(s1Rows[0].platform_id).toBe(B);
|
||||
expect(JSON.parse(s1Rows[0].content).text).toBe('pong');
|
||||
expect(s2Rows).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('fallback: a2a with no in_reply_to falls through to newest-session lookup', async () => {
|
||||
// No prior conversation. B initiates an a2a to A out of the blue.
|
||||
await routeAgentMessage(
|
||||
{
|
||||
id: 'msg-from-B-fresh',
|
||||
platform_id: A,
|
||||
content: JSON.stringify({ text: 'unsolicited' }),
|
||||
in_reply_to: null,
|
||||
},
|
||||
SB,
|
||||
);
|
||||
|
||||
// Newest session wins (current heuristic, preserved).
|
||||
const s1Rows = readInbound(A, S1.id);
|
||||
const s2Rows = readInbound(A, S2.id);
|
||||
expect(s1Rows).toHaveLength(0);
|
||||
expect(s2Rows).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('peer-affinity fallback: with no in_reply_to, routes to most recent peer-source session', async () => {
|
||||
// A.S1 sends to B (establishing affinity: B's last contact from A was via S1).
|
||||
await routeAgentMessage(
|
||||
{
|
||||
id: 'msg-from-A-S1-pre',
|
||||
platform_id: B,
|
||||
content: JSON.stringify({ text: 'context-establishing' }),
|
||||
in_reply_to: null,
|
||||
},
|
||||
S1,
|
||||
);
|
||||
|
||||
// B sends a follow-up but its container forgot to set in_reply_to (e.g.
|
||||
// emitted via an MCP tool path that doesn't thread the batch's in_reply_to
|
||||
// through). The host should still route this to S1 because S1 is the
|
||||
// session most recently in conversation with B — not the chronologically
|
||||
// newest session of A.
|
||||
await routeAgentMessage(
|
||||
{
|
||||
id: 'msg-from-B-followup',
|
||||
platform_id: A,
|
||||
content: JSON.stringify({ text: 'standing by' }),
|
||||
in_reply_to: null,
|
||||
},
|
||||
SB,
|
||||
);
|
||||
|
||||
const s1Rows = readInbound(A, S1.id);
|
||||
const s2Rows = readInbound(A, S2.id);
|
||||
// Affinity wins: reply to S1, not the newer S2.
|
||||
expect(s1Rows).toHaveLength(1);
|
||||
expect(JSON.parse(s1Rows[0].content).text).toBe('standing by');
|
||||
expect(s2Rows).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -23,10 +23,11 @@ import path from 'path';
|
||||
|
||||
import { isSafeAttachmentName } from '../../attachment-safety.js';
|
||||
import { getAgentGroup } from '../../db/agent-groups.js';
|
||||
import { getInboundSourceSessionId, getMostRecentPeerSourceSessionId } from '../../db/session-db.js';
|
||||
import { getSession } from '../../db/sessions.js';
|
||||
import { wakeContainer } from '../../container-runner.js';
|
||||
import { log } from '../../log.js';
|
||||
import { resolveSession, sessionDir, writeSessionMessage } from '../../session-manager.js';
|
||||
import { openInboundDb, resolveSession, sessionDir, writeSessionMessage } from '../../session-manager.js';
|
||||
import type { Session } from '../../types.js';
|
||||
import { hasDestination } from './db/agent-destinations.js';
|
||||
|
||||
@@ -101,6 +102,61 @@ export interface RoutableAgentMessage {
|
||||
id: string;
|
||||
platform_id: string | null;
|
||||
content: string;
|
||||
/**
|
||||
* For replies, the id of the inbound message being replied to. The
|
||||
* container's formatter sets this from the first inbound in the batch
|
||||
* (`container/agent-runner/src/formatter.ts`). Used here to route the
|
||||
* reply back to the originating session — see `resolveTargetSession`.
|
||||
*/
|
||||
in_reply_to: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Pick which session of `targetAgentGroupId` should receive this a2a message.
|
||||
*
|
||||
* Three layers, highest-fidelity first:
|
||||
*
|
||||
* 1. **Direct return-path** (in_reply_to lookup): if the message is a reply
|
||||
* (`in_reply_to` set), open the source agent's inbound DB and read the
|
||||
* triggering row's `source_session_id`. That column was stamped when the
|
||||
* original outbound was routed — it's the session that started the
|
||||
* conversation, and replies should land there even when the target has
|
||||
* multiple active sessions.
|
||||
*
|
||||
* 2. **Peer-affinity fallback**: if (1) misses (in_reply_to is null or the
|
||||
* referenced row isn't an a2a inbound), look up the most recent a2a
|
||||
* inbound *from the target agent group* in source's inbound and use its
|
||||
* `source_session_id`. The intuition: the last time this peer talked to
|
||||
* me, which target session was driving? Route the reply there, since
|
||||
* that's the session most plausibly in active conversation.
|
||||
*
|
||||
* 3. **Newest active session**: legacy heuristic. Used when no prior a2a
|
||||
* has been recorded with `source_session_id` (e.g. fresh installs,
|
||||
* pre-migration data).
|
||||
*/
|
||||
function resolveTargetSession(msg: RoutableAgentMessage, sourceSession: Session, targetAgentGroupId: string): Session {
|
||||
const srcDb = openInboundDb(sourceSession.agent_group_id, sourceSession.id);
|
||||
let originSessionId: string | null = null;
|
||||
try {
|
||||
if (msg.in_reply_to) {
|
||||
originSessionId = getInboundSourceSessionId(srcDb, msg.in_reply_to);
|
||||
}
|
||||
if (!originSessionId) {
|
||||
// Peer-affinity fallback — covers the case where the container's
|
||||
// outbound write didn't carry in_reply_to (e.g. legacy MCP send_message
|
||||
// path, container running pre-fix code).
|
||||
originSessionId = getMostRecentPeerSourceSessionId(srcDb, targetAgentGroupId);
|
||||
}
|
||||
} finally {
|
||||
srcDb.close();
|
||||
}
|
||||
if (originSessionId) {
|
||||
const candidate = getSession(originSessionId);
|
||||
if (candidate && candidate.agent_group_id === targetAgentGroupId && candidate.status === 'active') {
|
||||
return candidate;
|
||||
}
|
||||
}
|
||||
return resolveSession(targetAgentGroupId, null, null, 'agent-shared').session;
|
||||
}
|
||||
|
||||
export async function routeAgentMessage(msg: RoutableAgentMessage, session: Session): Promise<void> {
|
||||
@@ -119,7 +175,7 @@ export async function routeAgentMessage(msg: RoutableAgentMessage, session: Sess
|
||||
if (!getAgentGroup(targetAgentGroupId)) {
|
||||
throw new Error(`target agent group ${targetAgentGroupId} not found for message ${msg.id}`);
|
||||
}
|
||||
const { session: targetSession } = resolveSession(targetAgentGroupId, null, null, 'agent-shared');
|
||||
const targetSession = resolveTargetSession(msg, session, targetAgentGroupId);
|
||||
const a2aMsgId = `a2a-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
|
||||
|
||||
// If the source message references files (via `send_file`), forward the
|
||||
@@ -137,6 +193,7 @@ export async function routeAgentMessage(msg: RoutableAgentMessage, session: Sess
|
||||
channelType: 'agent',
|
||||
threadId: null,
|
||||
content: forwardedContent,
|
||||
sourceSessionId: session.id,
|
||||
});
|
||||
log.info('Agent message routed', {
|
||||
from: session.agent_group_id,
|
||||
|
||||
Reference in New Issue
Block a user