From 4ee5743382bfa18e770365ec40c12408cf6f0157 Mon Sep 17 00:00:00 2001 From: Relayflow Date: Thu, 1 Oct 2026 20:42:48 +0000 Subject: [PATCH 1/2] fix(mcp): forward DM idempotency keys across processes Align RelayAgentThinClient.dm with the sibling relaycast-client.ts option. Cover the MCP forwarding contract and session-stamping proxy, and prove base/head behavior with fresh processes against a local HTTP service. --- .../compact_4n7puk2lhmzy_2026-10-01.json | 37 +++++ .../compact_4n7puk2lhmzy_2026-10-01.md | 18 +++ .../2026-10/traj_degjzqu0skub/compaction.json | 5 + .../2026-10/traj_degjzqu0skub/summary.md | 32 ++++ .../2026-10/traj_degjzqu0skub/trajectory.json | 57 +++++++ CHANGELOG.md | 2 + .../src/cli/agent-relay-mcp.startup.test.ts | 3 + .../cli/mcp/messaging-tools.protocol.test.ts | 53 +++++++ packages/cli/src/cli/mcp/messaging-tools.ts | 1 + .../sdk/src/__tests__/thin-client.test.ts | 14 +- packages/sdk/src/messaging/thin-client.ts | 2 + .../cases/1874-mcp-dm-idempotency/case.json | 13 ++ .../1874-mcp-dm-idempotency/probe.test.mts | 47 ++++++ .../cases/1874-mcp-dm-idempotency/run.mjs | 147 ++++++++++++++++++ 14 files changed, 428 insertions(+), 3 deletions(-) create mode 100644 .agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.json create mode 100644 .agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.md create mode 100644 .agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/compaction.json create mode 100644 .agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/summary.md create mode 100644 .agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/trajectory.json create mode 100644 tests/relayflows/cases/1874-mcp-dm-idempotency/case.json create mode 100644 tests/relayflows/cases/1874-mcp-dm-idempotency/probe.test.mts create mode 100644 tests/relayflows/cases/1874-mcp-dm-idempotency/run.mjs diff --git a/.agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.json b/.agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.json new file mode 100644 index 0000000000..68286ce4e5 --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.json @@ -0,0 +1,37 @@ +{ + "id": "compact_4n7puk2lhmzy", + "version": 1, + "type": "compacted", + "compactedAt": "2026-10-01T20:41:58.659Z", + "sourceTrajectories": [ + "traj_degjzqu0skub" + ], + "dateRange": { + "start": "2026-10-01T20:40:14.887Z", + "end": "2026-10-01T20:41:58.282Z" + }, + "summary": { + "totalDecisions": 1, + "totalEvents": 1, + "uniqueAgents": [ + "default" + ] + }, + "decisionGroups": [ + { + "category": "api", + "decisions": [ + { + "question": "Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct", + "chosen": "Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct", + "reasoning": "Re-land the focused production fix from PR 1866. Rebuild the proof with separate processes and a real SDK HTTP path; derive outcomes from observations instead of the arm.", + "fromTrajectory": "traj_degjzqu0skub" + } + ] + } + ], + "keyLearnings": [], + "keyFindings": [], + "filesAffected": [], + "commits": [] +} \ No newline at end of file diff --git a/.agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.md b/.agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.md new file mode 100644 index 0000000000..b17848caa7 --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency/compacted/compact_4n7puk2lhmzy_2026-10-01.md @@ -0,0 +1,18 @@ +# Trajectory Compaction: Oct 1, 2026 - Oct 1, 2026 + +## Summary +- Sessions: 1 +- Decisions: 1 +- Events: 1 +- Agents: default +- Files: 0 +- Commits: 0 + +## Api +- Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct -> Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct (traj_degjzqu0skub) + +## Key Learnings +- None + +## Key Findings +- None \ No newline at end of file diff --git a/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/compaction.json b/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/compaction.json new file mode 100644 index 0000000000..8224714cac --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/compaction.json @@ -0,0 +1,5 @@ +{ + "trajectoryId": "traj_degjzqu0skub", + "compactedInto": "compact_4n7puk2lhmzy", + "compactedAt": "2026-10-01T20:41:58.673Z" +} \ No newline at end of file diff --git a/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/summary.md b/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/summary.md new file mode 100644 index 0000000000..fab5839fde --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/summary.md @@ -0,0 +1,32 @@ +# Trajectory: Forward MCP DM idempotency across process boundaries + +> **Status:** ✅ Completed +> **Task:** 1874 +> **Confidence:** 90% +> **Started:** October 1, 2026 at 08:40 PM +> **Completed:** October 1, 2026 at 08:41 PM + +--- + +## Summary + +Forwarded explicit MCP DM keys through raw and session-stamped SDK clients. Protocol/startup 67 tests, SDK 180 tests, six type tests, typecheck and lint pass. Base proof creates two keyed rows; patched proof returns one row and identical receipts; unkeyed sends stay distinct. Two unrelated suite failures reproduce on base, and supplied plan files fail formatting. No live recipient injection check. + +**Approach:** Standard approach + +--- + +## Key Decisions + +### Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct +- **Chose:** Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct +- **Reasoning:** Re-land the focused production fix from PR 1866. Rebuild the proof with separate processes and a real SDK HTTP path; derive outcomes from observations instead of the arm. + +--- + +## Chapters + +### 1. Work +*Agent: default* + +- Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct: Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct diff --git a/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/trajectory.json b/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/trajectory.json new file mode 100644 index 0000000000..372f23a75a --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency/completed/2026-10/traj_degjzqu0skub/trajectory.json @@ -0,0 +1,57 @@ +{ + "id": "traj_degjzqu0skub", + "version": 1, + "task": { + "title": "Forward MCP DM idempotency across process boundaries", + "source": { + "system": "plain", + "id": "1874" + } + }, + "status": "completed", + "startedAt": "2026-10-01T20:40:14.887Z", + "completedAt": "2026-10-01T20:41:58.282Z", + "agents": [ + { + "name": "default", + "role": "lead", + "joinedAt": "2026-10-01T20:40:15.338Z" + } + ], + "chapters": [ + { + "id": "chap_7inqsnzzfjjh", + "title": "Work", + "agentName": "default", + "startedAt": "2026-10-01T20:40:15.338Z", + "endedAt": "2026-10-01T20:41:58.282Z", + "events": [ + { + "ts": 1790887215339, + "type": "decision", + "content": "Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct: Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct", + "raw": { + "question": "Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct", + "chosen": "Forward only explicit DM keys, matching relaycast-client.ts; keep unkeyed sends distinct", + "alternatives": [], + "reasoning": "Re-land the focused production fix from PR 1866. Rebuild the proof with separate processes and a real SDK HTTP path; derive outcomes from observations instead of the arm." + }, + "significance": "high" + } + ] + } + ], + "retrospective": { + "summary": "Forwarded explicit MCP DM keys through raw and session-stamped SDK clients. Protocol/startup 67 tests, SDK 180 tests, six type tests, typecheck and lint pass. Base proof creates two keyed rows; patched proof returns one row and identical receipts; unkeyed sends stay distinct. Two unrelated suite failures reproduce on base, and supplied plan files fail formatting. No live recipient injection check.", + "approach": "Standard approach", + "confidence": 0.9 + }, + "commits": [], + "filesChanged": [], + "projectId": "AgentWorkforce/relay", + "tags": [], + "_trace": { + "startRef": "35e7cdedad77fb6c5d4c2a15f0ee4537cb0f4b3b", + "endRef": "35e7cdedad77fb6c5d4c2a15f0ee4537cb0f4b3b" + } +} \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e9757323a..1c7815c02d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- MCP `send_dm` now forwards its explicit idempotency key to Relaycast, so a retry remains a single direct message even when it outlives the process-local replay cache. + - Broker: Muse workers now reuse the host Muse login (`RELAY_MUSE_SHARED_AUTH_PATH`, the host's `MUSE_AUTH_PATH`, `$XDG_CONFIG_HOME/muse/auth.json`, or `$HOME/.config/muse/auth.json`) instead of prompting for provider auth under every new agent name; Relay MCP settings stay per-worker. Multi-tenant hosts should set `RELAY_MUSE_ISOLATED_AUTH=1` before upgrading to keep a separate login per worker. ## [13.0.0] - 2026-09-30 diff --git a/packages/cli/src/cli/agent-relay-mcp.startup.test.ts b/packages/cli/src/cli/agent-relay-mcp.startup.test.ts index f645936188..96d615bc59 100644 --- a/packages/cli/src/cli/agent-relay-mcp.startup.test.ts +++ b/packages/cli/src/cli/agent-relay-mcp.startup.test.ts @@ -545,6 +545,9 @@ describe('agent-relay-mcp startup helpers', () => { expect(retry).toEqual(first); expect(mocks.agentClients.reduce((count, client) => count + client.dm.mock.calls.length, 0)).toBe(1); + expect(mocks.agentClients.flatMap((client) => client.dm.mock.calls)).toEqual([ + [input.to, input.text, expect.objectContaining({ idempotencyKey: input.idempotency_key })], + ]); }); it('parses startup options and helper flags from the environment', async () => { diff --git a/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts b/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts index 5e48b45fff..abb535f8e3 100644 --- a/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts +++ b/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts @@ -3,6 +3,8 @@ import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { McpRequestReplay } from './request-replay.js'; + import { registerMessagingTools } from './messaging-tools.js'; describe('messaging delivery receipts over MCP', () => { @@ -14,6 +16,57 @@ describe('messaging delivery receipts over MCP', () => { vi.unstubAllEnvs(); }); + it('reuses keyed receipts across two replay boundaries while unkeyed identical sends stay distinct', async () => { + const rows: { id: string; conversationId: string }[] = []; + const keyed = new Map(); + const dm = vi.fn(async (_to: string, _text: string, options: { idempotencyKey?: string }) => { + const key = options.idempotencyKey; + if (key && keyed.has(key)) return keyed.get(key)!; + const row = { id: `msg_${rows.length + 1}`, conversationId: 'dm_chief' }; + rows.push(row); + if (key) keyed.set(key, row); + return row; + }); + async function send(idempotencyKey?: string) { + const server = new McpServer({ name: 'boundary-test', version: '1.0.0' }); + registerMessagingTools( + server, + () => ({ dm }) as never, + async () => [{ name: 'chief' }], + new McpRequestReplay() + ); + const client = new Client({ name: 'boundary-client', version: '1.0.0' }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + try { + await server.connect(serverTransport); + await client.connect(clientTransport); + const result = await client.callTool({ + name: 'send_dm', + arguments: { + to: 'chief', + text: 'same text', + ...(idempotencyKey ? { idempotency_key: idempotencyKey } : {}), + }, + }); + expect(result.isError).not.toBe(true); + return result.structuredContent; + } finally { + await client.close(); + await server.close(); + } + } + const first = await send('logical-send'); + const retry = await send('logical-send'); + expect(rows).toHaveLength(1); + expect(first).toMatchObject({ id: 'msg_1', conversationId: 'dm_chief' }); + expect(retry).toEqual(first); + expect(dm).toHaveBeenCalledTimes(2); + const unkeyedFirst = await send(); + const unkeyedSecond = await send(); + expect(rows).toHaveLength(3); + expect(unkeyedFirst?.id).not.toBe(unkeyedSecond?.id); + }); + it('exposes enqueue state on send and an explicit signal for an empty reader list', async () => { const dm = vi.fn(async () => ({ id: 'msg_1', text: 'hello' })); const readers = vi.fn(async () => []); diff --git a/packages/cli/src/cli/mcp/messaging-tools.ts b/packages/cli/src/cli/mcp/messaging-tools.ts index 46c7981138..cee3881b56 100644 --- a/packages/cli/src/cli/mcp/messaging-tools.ts +++ b/packages/cli/src/cli/mcp/messaging-tools.ts @@ -371,6 +371,7 @@ export function registerMessagingTools( const agents = await listAgentsForRecipientResolution?.(); const resolvedRecipient = agents ? resolveExactAgentName(agents, to) : undefined; const message = await getAgentClient(as).dm(to, text, { + idempotencyKey: idempotency_key, mode, attachments, data: replayMessageMetadata(), diff --git a/packages/sdk/src/__tests__/thin-client.test.ts b/packages/sdk/src/__tests__/thin-client.test.ts index 62f7fe2465..f7851158ed 100644 --- a/packages/sdk/src/__tests__/thin-client.test.ts +++ b/packages/sdk/src/__tests__/thin-client.test.ts @@ -199,10 +199,15 @@ describe('createAgentClient', () => { }); expect(instance.as).toHaveBeenCalledWith('at_live_test', { autoHeartbeatMs: false }); - const agent = relaycastMocks.agentClients[0] as { send: ReturnType }; + const agent = relaycastMocks.agentClients[0] as { + send: ReturnType; + dm: ReturnType; + }; const sent = await client.send('general', 'hello', { mode: 'wait' }); expect(agent.send).toHaveBeenCalledWith('general', 'hello', { mode: 'wait' }); expect(sent).toEqual({ raw: true, args: ['general', 'hello', { mode: 'wait' }] }); + await client.dm('chief', 'dm', { idempotencyKey: 'dm-logical-1' }); + expect(agent.dm).toHaveBeenCalledWith('chief', 'dm', { idempotencyKey: 'dm-logical-1' }); }); it('honors an explicit heartbeat interval', () => { @@ -239,13 +244,16 @@ describe('createAgentClient', () => { await client.send('general', 'hello'); await client.reply('msg_parent', 'reply'); - await client.dm('chief', 'dm'); + await client.dm('chief', 'dm', { idempotencyKey: 'dm-logical-1' }); await client.dms.sendMessage('conv_group', 'group'); const replayData = { session_ref: '11111111-1111-4111-8111-111111111111' }; expect(agent.send).toHaveBeenCalledWith('general', 'hello', { data: replayData }); expect(agent.reply).toHaveBeenCalledWith('msg_parent', 'reply', { data: replayData }); - expect(agent.dm).toHaveBeenCalledWith('chief', 'dm', { data: replayData }); + expect(agent.dm).toHaveBeenCalledWith('chief', 'dm', { + data: replayData, + idempotencyKey: 'dm-logical-1', + }); expect(agent.dms.sendMessage).toHaveBeenCalledWith('conv_group', 'group', { data: replayData }); }); diff --git a/packages/sdk/src/messaging/thin-client.ts b/packages/sdk/src/messaging/thin-client.ts index 087b457264..97e225e13b 100644 --- a/packages/sdk/src/messaging/thin-client.ts +++ b/packages/sdk/src/messaging/thin-client.ts @@ -119,6 +119,8 @@ export interface RelayAgentThinClient { to: string, text: string, options?: { + /** Stable key for retries of one logical send, including across processes. */ + idempotencyKey?: string; mode?: RelayMessageMode; attachments?: string[]; data?: Record | null; diff --git a/tests/relayflows/cases/1874-mcp-dm-idempotency/case.json b/tests/relayflows/cases/1874-mcp-dm-idempotency/case.json new file mode 100644 index 0000000000..b2d80a071f --- /dev/null +++ b/tests/relayflows/cases/1874-mcp-dm-idempotency/case.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "id": "1874-mcp-dm-idempotency", + "kind": "bugfix", + "title": "Preserve DM idempotency across fresh MCP processes", + "requirements": [], + "runner": { "command": ["node", "tests/relayflows/cases/1874-mcp-dm-idempotency/run.mjs"] }, + "timeoutSeconds": 1200, + "expected": { + "base": { "outcome": "bug", "signature": "mcp_retry_creates_two_direct_messages" }, + "head": { "outcome": "fixed", "signature": "mcp_retry_reuses_one_direct_message" } + } +} diff --git a/tests/relayflows/cases/1874-mcp-dm-idempotency/probe.test.mts b/tests/relayflows/cases/1874-mcp-dm-idempotency/probe.test.mts new file mode 100644 index 0000000000..e9380d9938 --- /dev/null +++ b/tests/relayflows/cases/1874-mcp-dm-idempotency/probe.test.mts @@ -0,0 +1,47 @@ +import { writeFile } from 'node:fs/promises'; +import { pathToFileURL } from 'node:url'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import { it } from 'vitest'; + +it('records a send through a fresh MCP replay cache and the real Relaycast SDK', async () => { + const target = process.env.RELAY_PR_PROOF_TARGET_DIR!; + const { registerMessagingTools } = await import( + pathToFileURL(`${target}/packages/cli/src/cli/mcp/messaging-tools.ts`).href + ); + const { McpRequestReplay } = await import( + pathToFileURL(`${target}/packages/cli/src/cli/mcp/request-replay.ts`).href + ); + const { createAgentClient } = await import( + pathToFileURL(`${target}/packages/sdk/src/messaging/thin-client.ts`).href + ); + const agent = createAgentClient({ agentToken: 'at_local_proof', baseUrl: process.env.DM_PROOF_BASE_URL! }); + const server = new McpServer({ name: 'dm-proof', version: '1.0.0' }); + registerMessagingTools( + server, + () => agent, + async () => [{ name: 'chief' }], + new McpRequestReplay() + ); + const client = new Client({ name: 'dm-proof-client', version: '1.0.0' }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + try { + await server.connect(serverTransport); + await client.connect(clientTransport); + const result = await client.callTool({ + name: 'send_dm', + arguments: { + to: 'chief', + text: 'process-boundary-proof', + ...(process.env.DM_PROOF_KEY ? { idempotency_key: process.env.DM_PROOF_KEY } : {}), + }, + }); + if (result.isError || !result.structuredContent) + throw new Error(`send_dm failed: ${JSON.stringify(result)}`); + await writeFile(process.env.DM_PROOF_RECEIPT!, JSON.stringify(result.structuredContent)); + } finally { + await client.close(); + await server.close(); + } +}); diff --git a/tests/relayflows/cases/1874-mcp-dm-idempotency/run.mjs b/tests/relayflows/cases/1874-mcp-dm-idempotency/run.mjs new file mode 100644 index 0000000000..470826de60 --- /dev/null +++ b/tests/relayflows/cases/1874-mcp-dm-idempotency/run.mjs @@ -0,0 +1,147 @@ +import { execFileSync, spawn } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { copyFile, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import http from 'node:http'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const caseId = '1874-mcp-dm-idempotency'; +const required = (name) => { + if (!process.env[name]) throw new Error(`Missing ${name}`); + return process.env[name]; +}; +const targetDir = path.resolve(required('RELAY_PR_PROOF_TARGET_DIR')); +const harnessDir = path.resolve(required('RELAY_PR_PROOF_HARNESS_DIR')); +const resultPath = required('RELAY_PR_PROOF_RESULT_PATH'); +const arm = required('RELAY_PR_PROOF_ARM'); +if (!['base', 'head'].includes(arm)) throw new Error(`Invalid arm: ${arm}`); +const expectedSha = required(arm === 'base' ? 'RELAY_PR_PROOF_BASE_SHA' : 'RELAY_PR_PROOF_HEAD_SHA'); +const actualSha = execFileSync('git', ['-C', targetDir, 'rev-parse', 'HEAD'], { encoding: 'utf8' }).trim(); +if (actualSha !== expectedSha) throw new Error(`Target ${actualSha} differs from expected ${expectedSha}`); +const runnerPath = fileURLToPath(import.meta.url); +const relative = path.relative(harnessDir, runnerPath); +if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { + throw new Error('Runner must execute from the exact-head harness checkout'); +} + +// Async children leave the event loop free to serve the real SDK's HTTP requests. +async function run(command, args, env = {}) { + await new Promise((resolve, reject) => { + const child = spawn(command, args, { + cwd: targetDir, + env: { ...process.env, ...env }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let tail = ''; + for (const stream of [child.stdout, child.stderr]) + stream.on('data', (chunk) => { + tail = (tail + chunk.toString()).slice(-6000); + }); + const timer = setTimeout(() => child.kill('SIGKILL'), 600_000); + child.once('error', (error) => { + clearTimeout(timer); + reject(error); + }); + child.once('exit', (code, signal) => { + clearTimeout(timer); + if (code === 0) resolve(); + else reject(new Error(`${command} exited ${signal ?? code}: ${tail}`)); + }); + }); +} + +if (!existsSync(path.join(targetDir, 'node_modules/vitest/vitest.mjs'))) { + await run('npm', ['ci', '--ignore-scripts', '--no-audit', '--no-fund']); +} +await run('npm', ['run', 'build:sdk']); +const probeDir = await mkdtemp(path.join(targetDir, '.dm-proof-')); +const rows = []; +const keyed = new Map(); +const requests = []; +let serverError; +const server = http.createServer(async (request, response) => { + try { + if (request.method !== 'POST' || request.url !== '/v1/dm') + throw new Error(`Unexpected route: ${request.method} ${request.url}`); + let body = ''; + for await (const chunk of request) body += chunk; + const parsed = JSON.parse(body); + if ('idempotencyKey' in parsed || 'idempotency_key' in parsed) throw new Error('Key leaked into body'); + const key = request.headers['idempotency-key']; + let row = key && keyed.get(key); + if (!row) { + row = { id: `msg_${rows.length + 1}`, conversation_id: 'dm_chief' }; + rows.push(row); + if (key) keyed.set(key, row); + } + requests.push({ key, id: row.id }); + response.writeHead(200, { 'content-type': 'application/json' }); + response.end(JSON.stringify({ ok: true, data: row })); + } catch (error) { + serverError = error; + response.writeHead(500).end(); + } +}); +try { + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', resolve); + }); + const probePath = path.join(probeDir, 'probe.test.mts'); + await copyFile(path.join(path.dirname(runnerPath), 'probe.test.mts'), probePath); + const configPath = path.join(probeDir, 'vitest.config.mts'); + // Preserve the target's workspace source aliases, overriding only test discovery. + await writeFile( + configPath, + `import config from '../vitest.config.ts';\nexport default { ...config, test: { ...config.test, include: [${JSON.stringify(probePath)}], exclude: [], setupFiles: [] } };\n` + ); + async function attempt(index, key) { + const receiptPath = path.join(probeDir, `receipt-${index}.json`); + await run( + process.execPath, + [path.join(targetDir, 'node_modules/vitest/vitest.mjs'), 'run', '--config', configPath], + { + RELAY_PR_PROOF_TARGET_DIR: targetDir, + RELAY_ATTEST_SESSION_ID: '11111111-1111-4111-8111-111111111111', + DM_PROOF_BASE_URL: `http://127.0.0.1:${server.address().port}`, + DM_PROOF_KEY: key, + DM_PROOF_RECEIPT: receiptPath, + } + ); + const receipt = JSON.parse(await readFile(receiptPath, 'utf8')); + if (!receipt.id || receipt.conversationId !== 'dm_chief') + throw new Error(`Invalid receipt: ${JSON.stringify(receipt)}`); + return receipt; + } + const first = await attempt(1, 'logical-dm-proof'); + const retry = await attempt(2, 'logical-dm-proof'); + const keyedRows = rows.length; + const keyedRequests = requests.slice(); + const unkeyedFirst = await attempt(3, ''); + const unkeyedSecond = await attempt(4, ''); + if (serverError) throw serverError; + if (rows.length !== keyedRows + 2 || unkeyedFirst.id === unkeyedSecond.id || requests.length !== 4) { + throw new Error(`Unexpected unkeyed observations: ${JSON.stringify({ rows, requests })}`); + } + const receiptsMatchRows = first.id === keyedRequests[0]?.id && retry.id === keyedRequests[1]?.id; + const bug = keyedRows === 2 && first.id !== retry.id; + const fixed = + keyedRows === 1 && first.id === retry.id && keyedRequests.every(({ key }) => key === 'logical-dm-proof'); + if (!receiptsMatchRows || (!bug && !fixed)) + throw new Error(`Unexpected keyed observations: ${JSON.stringify({ rows, first, retry, requests })}`); + await mkdir(path.dirname(resultPath), { recursive: true }); + await writeFile( + resultPath, + JSON.stringify({ + version: 1, + caseId, + arm, + outcome: fixed ? 'fixed' : 'bug', + signature: fixed ? 'mcp_retry_reuses_one_direct_message' : 'mcp_retry_creates_two_direct_messages', + details: `Fresh processes created ${keyedRows} keyed row(s); receipt IDs ${first.id}, ${retry.id}; conversation ${first.conversationId}. Two unkeyed sends created two more rows. Local HTTP service models upstream idempotency; recipient injection is not exercised.`, + }) + '\n' + ); +} finally { + await new Promise((resolve) => server.close(resolve)); + await rm(probeDir, { recursive: true, force: true }); +} From 040e655a245c48210afadead1dd0bb035e98e3aa Mon Sep 17 00:00:00 2001 From: Relayflow Date: Thu, 1 Oct 2026 20:53:44 +0000 Subject: [PATCH 2/2] fix(mcp): reject whitespace-only DM idempotency keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relaycast trims the idempotency key upstream and falls back to a generated key when the trimmed value is empty, so a whitespace-only send_dm key passed the local replay cache as an explicit stable key while reaching Relaycast as a fresh key in every process — storing a second message on retry. Trim the key at the schema boundary so the replay cache and the forwarded call agree with upstream normalization, and reject a key that is empty once trimmed. Co-Authored-By: Claude Opus 5 --- .../compact_bbjujvsvrew2_2026-10-01.json | 76 +++++++++++++++++++ .../compact_bbjujvsvrew2_2026-10-01.md | 27 +++++++ CHANGELOG.md | 2 +- .../cli/mcp/messaging-tools.protocol.test.ts | 37 +++++++++ packages/cli/src/cli/mcp/messaging-tools.ts | 7 +- 5 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 .agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.json create mode 100644 .agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.md diff --git a/.agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.json b/.agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.json new file mode 100644 index 0000000000..ad631409ac --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.json @@ -0,0 +1,76 @@ +{ + "id": "compact_bbjujvsvrew2", + "version": 1, + "type": "compacted", + "compactedAt": "2026-10-01T20:47:55.402Z", + "sourceTrajectories": [ + "traj_cb6pd2x3am02" + ], + "dateRange": { + "start": "2026-10-01T20:47:02.549Z", + "end": "2026-10-01T20:47:55.282Z" + }, + "summary": { + "totalDecisions": 1, + "totalEvents": 1, + "uniqueAgents": [ + "default" + ] + }, + "decisionGroups": [ + { + "category": "other", + "decisions": [ + { + "question": "Withhold clean review for accepted whitespace-only idempotency keys", + "chosen": "Withhold clean review for accepted whitespace-only idempotency keys", + "reasoning": "The real SDK trims the key to empty and generates a random key per attempt. The fresh-process proof returns msg_1 then msg_2 and two rows on head for a three-space key.", + "fromTrajectory": "traj_cb6pd2x3am02" + } + ] + } + ], + "keyLearnings": [], + "keyFindings": [], + "filesAffected": [], + "commits": [], + "narrative": "Reviewed PR #1875 (DM idempotency key forwarding) and identified a P2 defect where whitespace-only idempotency keys are accepted but produce non-idempotent behavior. Testing showed the real SDK trims the key to empty string and generates a random key per attempt, causing a fresh-process proof to return msg_1 then msg_2 with two distinct rows in the database for a three-space key input. All automated tests passed (67 MCP tests, 180 SDK tests, 6 type tests, typecheck), but the whitespace-only key edge case was documented in review.md without issuing a clean review approval. The review session established that passing automated tests is necessary but not sufficient for clean approval when edge-case behavior violates idempotency guarantees. The decision to withhold approval was based on observing real-world SDK behavior through fresh-process proofs, demonstrating that empirical validation catches semantic defects that unit tests may miss. No code changes were made during this review; the finding was documented for the PR author to address.", + "decisions": [ + { + "question": "Should PR #1875 receive clean approval despite passing all automated tests?", + "chosen": "Withhold clean review approval and document P2 defect for whitespace-only idempotency keys", + "reasoning": "The real SDK trims whitespace-only keys to empty string and generates random keys per attempt, causing two database rows for a three-space key. This violates the idempotency contract even though all 67 MCP tests, 180 SDK tests, and type tests passed. The fresh-process proof empirically demonstrated non-idempotent behavior.", + "impact": "Prevents merging code with a semantic defect in idempotency guarantees. Establishes that empirical fresh-process proofs can reveal edge-case failures missed by unit tests. PR author must address whitespace handling before approval." + } + ], + "conventions": [ + { + "pattern": "Use fresh-process proofs to validate idempotency claims empirically, not just automated test suite results", + "rationale": "All 253 automated tests passed, but a fresh-process proof with a whitespace-only key exposed that the SDK generates random keys per attempt, creating duplicate database rows. Unit tests missed this edge case.", + "scope": "PR reviews for idempotency features, DM forwarding, and SDK protocol correctness" + }, + { + "pattern": "Document P2 defects in review.md and withhold clean approval even when tests pass", + "rationale": "Passing tests are necessary but not sufficient for approval. Edge-case semantic defects (like whitespace-only keys breaking idempotency) must be addressed before merge to prevent production issues.", + "scope": "All PR reviews, especially for protocol-level features like idempotency keys" + } + ], + "lessons": [ + { + "lesson": "Automated test suites can pass while missing critical edge cases in idempotency logic", + "context": "PR #1875 had 67 MCP tests, 180 SDK tests, and 6 type tests all passing, yet a manual fresh-process proof with a three-space idempotency key revealed the SDK trims to empty and generates random keys per attempt, producing two database rows instead of one.", + "recommendation": "Add automated tests for whitespace-only and empty-string idempotency keys. Extend the test suite to verify that trimmed/empty keys either reject or generate stable idempotent behavior, not random keys per attempt." + }, + { + "lesson": "Fresh-process proofs (restarting the SDK process between attempts) are essential for validating cross-process idempotency", + "context": "The defect only surfaced when running the SDK in separate processes (msg_1 then msg_2 with two database rows). In-process tests likely reused a cached key or state that masked the random key generation.", + "recommendation": "For idempotency features, always include at least one fresh-process integration test in the CI suite. Consider adding a test harness that spawns separate Node.js processes to verify cross-process behavior." + } + ], + "openQuestions": [ + "Should the SDK reject whitespace-only idempotency keys at the API boundary, or should it normalize them to a stable value?", + "Are there other input edge cases (null, undefined, very long strings) that should be validated in the idempotency key handling?", + "Should the MCP test suite include fresh-process proofs as standard practice for protocol-level features, or is that scope better suited for integration tests?", + "What is the correct behavior when an idempotency key trims to empty — generate a random key, reject the request, or treat it as no key provided?" + ] +} \ No newline at end of file diff --git a/.agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.md b/.agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.md new file mode 100644 index 0000000000..c63655a622 --- /dev/null +++ b/.agentworkforce/trajectories/dm-idempotency-review/compacted/compact_bbjujvsvrew2_2026-10-01.md @@ -0,0 +1,27 @@ +# Trajectory Compaction: 2026-10-01 - 2026-10-01 + +## Summary +Reviewed PR #1875 (DM idempotency key forwarding) and identified a P2 defect where whitespace-only idempotency keys are accepted but produce non-idempotent behavior. Testing showed the real SDK trims the key to empty string and generates a random key per attempt, causing a fresh-process proof to return msg_1 then msg_2 with two distinct rows in the database for a three-space key input. All automated tests passed (67 MCP tests, 180 SDK tests, 6 type tests, typecheck), but the whitespace-only key edge case was documented in review.md without issuing a clean review approval. The review session established that passing automated tests is necessary but not sufficient for clean approval when edge-case behavior violates idempotency guarantees. The decision to withhold approval was based on observing real-world SDK behavior through fresh-process proofs, demonstrating that empirical validation catches semantic defects that unit tests may miss. No code changes were made during this review; the finding was documented for the PR author to address. + +## Key Decisions (1) +| Question | Decision | Impact | +|----------|----------|--------| +| Should PR #1875 receive clean approval despite passing all automated tests? | Withhold clean review approval and document P2 defect for whitespace-only idempotency keys | Prevents merging code with a semantic defect in idempotency guarantees. Establishes that empirical fresh-process proofs can reveal edge-case failures missed by unit tests. PR author must address whitespace handling before approval. | + +## Conventions Established +- **Use fresh-process proofs to validate idempotency claims empirically, not just automated test suite results**: All 253 automated tests passed, but a fresh-process proof with a whitespace-only key exposed that the SDK generates random keys per attempt, creating duplicate database rows. Unit tests missed this edge case. (scope: PR reviews for idempotency features, DM forwarding, and SDK protocol correctness) +- **Document P2 defects in review.md and withhold clean approval even when tests pass**: Passing tests are necessary but not sufficient for approval. Edge-case semantic defects (like whitespace-only keys breaking idempotency) must be addressed before merge to prevent production issues. (scope: All PR reviews, especially for protocol-level features like idempotency keys) + +## Lessons Learned +- Automated test suites can pass while missing critical edge cases in idempotency logic (PR #1875 had 67 MCP tests, 180 SDK tests, and 6 type tests all passing, yet a manual fresh-process proof with a three-space idempotency key revealed the SDK trims to empty and generates random keys per attempt, producing two database rows instead of one.) - Add automated tests for whitespace-only and empty-string idempotency keys. Extend the test suite to verify that trimmed/empty keys either reject or generate stable idempotent behavior, not random keys per attempt. +- Fresh-process proofs (restarting the SDK process between attempts) are essential for validating cross-process idempotency (The defect only surfaced when running the SDK in separate processes (msg_1 then msg_2 with two database rows). In-process tests likely reused a cached key or state that masked the random key generation.) - For idempotency features, always include at least one fresh-process integration test in the CI suite. Consider adding a test harness that spawns separate Node.js processes to verify cross-process behavior. + +## Open Questions +- Should the SDK reject whitespace-only idempotency keys at the API boundary, or should it normalize them to a stable value? +- Are there other input edge cases (null, undefined, very long strings) that should be validated in the idempotency key handling? +- Should the MCP test suite include fresh-process proofs as standard practice for protocol-level features, or is that scope better suited for integration tests? +- What is the correct behavior when an idempotency key trims to empty — generate a random key, reject the request, or treat it as no key provided? + +## Stats +- Sessions: 1, Agents: default, Files: 0, Commits: 0 +- Date range: 2026-10-01T20:47:02.549Z - 2026-10-01T20:47:55.282Z \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c7815c02d..feb6dc2d96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- MCP `send_dm` now forwards its explicit idempotency key to Relaycast, so a retry remains a single direct message even when it outlives the process-local replay cache. +- MCP `send_dm` now forwards its explicit idempotency key to Relaycast, so a retry remains a single direct message even when it outlives the process-local replay cache. The key is trimmed to match Relaycast's own normalization, and a whitespace-only key is rejected instead of silently splitting the retry into a second message. - Broker: Muse workers now reuse the host Muse login (`RELAY_MUSE_SHARED_AUTH_PATH`, the host's `MUSE_AUTH_PATH`, `$XDG_CONFIG_HOME/muse/auth.json`, or `$HOME/.config/muse/auth.json`) instead of prompting for provider auth under every new agent name; Relay MCP settings stay per-worker. Multi-tenant hosts should set `RELAY_MUSE_ISOLATED_AUTH=1` before upgrading to keep a separate login per worker. diff --git a/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts b/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts index abb535f8e3..28d4b23576 100644 --- a/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts +++ b/packages/cli/src/cli/mcp/messaging-tools.protocol.test.ts @@ -67,6 +67,43 @@ describe('messaging delivery receipts over MCP', () => { expect(unkeyedFirst?.id).not.toBe(unkeyedSecond?.id); }); + it('rejects a whitespace-only idempotency key before sending and forwards it trimmed', async () => { + const dm = vi.fn(async () => ({ id: 'msg_1', conversationId: 'dm_chief' })); + const server = new McpServer({ name: 'idempotency-key-test', version: '1.0.0' }); + registerMessagingTools( + server, + () => ({ dm }) as never, + async () => [{ name: 'chief' }], + new McpRequestReplay() + ); + const client = new Client({ name: 'idempotency-key-client', version: '1.0.0' }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + try { + await server.connect(serverTransport); + await client.connect(clientTransport); + const rejected = await client.callTool({ + name: 'send_dm', + arguments: { to: 'chief', text: 'same text', idempotency_key: ' ' }, + }); + expect(rejected.isError).toBe(true); + expect(JSON.stringify(rejected.content)).toContain('idempotency_key'); + expect(dm).not.toHaveBeenCalled(); + const result = await client.callTool({ + name: 'send_dm', + arguments: { to: 'chief', text: 'same text', idempotency_key: ' logical-send ' }, + }); + expect(result.isError).not.toBe(true); + expect(dm).toHaveBeenCalledWith( + 'chief', + 'same text', + expect.objectContaining({ idempotencyKey: 'logical-send' }) + ); + } finally { + await client.close(); + await server.close(); + } + }); + it('exposes enqueue state on send and an explicit signal for an empty reader list', async () => { const dm = vi.fn(async () => ({ id: 'msg_1', text: 'hello' })); const readers = vi.fn(async () => []); diff --git a/packages/cli/src/cli/mcp/messaging-tools.ts b/packages/cli/src/cli/mcp/messaging-tools.ts index cee3881b56..dc74d1bbaf 100644 --- a/packages/cli/src/cli/mcp/messaging-tools.ts +++ b/packages/cli/src/cli/mcp/messaging-tools.ts @@ -350,11 +350,16 @@ export function registerMessagingTools( attachments: z.array(z.string()).optional().describe('File attachment IDs'), idempotency_key: z .string() + // Relaycast trims this key upstream, so trim before both the local + // replay cache and the forwarded call to keep one logical send on one + // key; a whitespace-only key would otherwise reach Relaycast as a + // freshly generated key per process and store a second message. + .trim() .min(1) .max(255) .optional() .describe( - 'Stable key for retrying this same message after a lost response; use a new key for a new message.' + 'Stable key for retrying this same message after a lost response; use a new key for a new message. Surrounding whitespace is trimmed and a whitespace-only key is rejected.' ), ...identityOverrideInputShape, },