Skip to content

MCP send_dm creates duplicate messages because idempotency_key stops at the process boundary - #1875

Closed
agent-relay-code[bot] wants to merge 2 commits into
mainfrom
relayflow/relay-software-garden-243c7cf6
Closed

agent-relay-code[bot] wants to merge 2 commits into
mainfrom
relayflow/relay-software-garden-243c7cf6

Conversation

@agent-relay-code

@agent-relay-code agent-relay-code Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #1874. Forward MCP send_dm's explicit idempotency_key through RelayAgentThinClient.dm to Relaycast, bringing the thin client into line with relaycast-client.ts. This makes the keyed retry path safe across MCP process boundaries; it does not establish what issued the second dispatch in the reported incident.

Re-lands the focused fix from #1866, head ba1ada05def104a6291f9a5f36698ee46df124f8. Related: AgentWorkforce/relay-desktop#100 and AgentWorkforce/relay-desktop#101. send_group_dm exposes no idempotency-key input, so there is no key to forward on that sibling path.

The issue's live trace also notes "earlier ordinary one-call sends from multiple
agents showed the same two-row/two-injection pattern" — i.e. unkeyed sends
duplicating. Those remain two rows after this change, by design, and the
acceptance criteria require that. Making a duplicate MCP dispatch safe without a
caller-supplied key is a different problem (a server-side or
argument-derived key), and is adjacent to the non-idempotent fleet-invocation
work tracked in #1604. This plan does not widen into it.

Test Plan

  • MCP protocol and startup: 67 tests pass. Both new forwarding regressions fail against base: two rows instead of one, and a missing SDK option.
  • SDK: 180 tests pass, including raw-client and replay-session proxy forwarding; six type tests pass.
  • npm run typecheck and npm run lint pass (lint reports existing warnings).
  • Separate-process proof: base exits zero with outcome: bug, two keyed rows and different message IDs; patched code exits zero with outcome: fixed, one keyed row and identical message/conversation IDs. Both preserve two distinct unkeyed sends.
  • Changed files pass Prettier and git diff --check.
  • Full root suite: initial run had 3,538 passed, 26 skipped, four failures. Two fleet-inventory failures cleared on rerun after dependency builds completed. The remaining sdk-client.test.ts and fleet-lifecycle-integration.test.ts failures reproduce on the unchanged base checkout.
  • Repository formatting: fails only on supplied plan.md and reviewed-plan.md; left unchanged.
  • Live production recipient injection: not re-run. fix(mcp): preserve DM idempotency upstream #1866 records two fresh MCP processes returning the same message/conversation IDs, one stored row, and one reader. The local proof uses the real SDK HTTP path and a local service modeling idempotency, not a live recipient; it does not attest production injection count or upstream collision/concurrency enforcement.

The prior PR's suppressed Devin Review flag could not be retrieved through GitHub comments/check APIs; no readable review link was returned. Its contents remain unverified.

RelayFlow Proof

  • Change type: bugfix
  • RelayFlow case: 1874-mcp-dm-idempotency

The harness runs a fresh Vitest/MCP process per attempt with the real replay cache, session-stamping proxy, and Relaycast SDK. A local HTTP server records rows and idempotency headers; the runner derives the outcome from those observations and verifies receipt IDs. No workflow files changed.

Screenshots

Not applicable.

Checks

Relayflow ran this repository's checks (.relayflow/check.sh) and they passed.

What ran (.relayflow/check.sh)
#!/bin/sh
# Fresh-machine CI parity check for this monorepo.
#
# Mirrors the jobs that gate a PR here:
#   .github/workflows/test.yml       -> test + lint jobs (Node/TypeScript)
#   .github/workflows/codegen-models.yml -> generated-model drift check
#   .github/workflows/rust-ci.yml    -> cargo test/clippy/fmt (toolchain-gated)
set -e

cd "$(dirname "$0")/.."

# test.yml sets this for every job.
AGENT_RELAY_TELEMETRY_DISABLED=1
export AGENT_RELAY_TELEMETRY_DISABLED

# --- Dependencies -----------------------------------------------------------
npm ci

# rollup's platform-specific optional dependency is missing from some npm
# installs; test.yml does the same best-effort install.
npm install --no-save rollup || true

# --- Code generation --------------------------------------------------------
# codegen-models.yml regenerates from packages/utils/cli-registry.yaml and
# fails the PR when the committed output has drifted.
npm run codegen:models
git diff --exit-code -- \
  packages/config/src/cli-registry.generated.ts \
  packages/sdk-py/src/agent_relay/models.py

# --- Build ------------------------------------------------------------------
# The same build `npm test` runs via its pretest hook. build:rust is a no-op
# without a cargo toolchain (published installs use the prebuilt platform
# optional dependencies), so this works on a Rust-less machine.
npm run build

# --- Lint -------------------------------------------------------------------
npm run lint
# Omitted: `npm run format:check`. It already fails at HEAD on pre-existing
# unformatted tracked files (plan.md, reviewed-plan.md), and
# prettier-fmt-fix.yml reformats on push, so it cannot gate local changes.

# --- Pre-suite guards (test.yml runs these before `npm test`) ---------------
node --test \
  scripts/pr-proof/broker-transfer.test.mjs \
  scripts/pr-proof/run-cloud.test.mjs \
  scripts/flows/deploy-listeners.test.mjs \
  scripts/pr-proof/event-from-input.test.mjs
npm run test:subscriptions:proof

# --- Tests ------------------------------------------------------------------
# `npm test` is `vitest run` behind a pretest hook that re-runs the clean +
# build above; call vitest directly to keep a single build.
npx vitest run

# --- Rust -------------------------------------------------------------------
# rust-ci.yml gates on crates/ changes. Only runnable where a toolchain is
# installed; this machine has no cargo, so these are skipped.
if command -v cargo >/dev/null 2>&1; then
  cargo test
  cargo clippy -- -D warnings
  cargo fmt -- --check
fi

# Deliberately left out (no secrets, services, or toolchains on this machine):
#   - e2e-tests.yml / fleet-e2e.yml / relay-evals.yml: need ANTHROPIC_API_KEY,
#     real agent CLIs, and Daytona sandboxes.
#   - prod-smoke.yml: hits production Relay endpoints.
#   - test.yml coverage job's Codecov upload: needs a Codecov token (the
#     coverage run itself is just `vitest run --coverage` over the same suite).
#   - test.yml windows-credential-acl job: windows-latest only.
#   - test.yml swift-test job and Package.swift build: need a Swift toolchain
#     (macOS runner).
#   - package-validation.yml publish/pack and standalone bun+macOS signing
#     jobs: need bun, publish-time dependency resolution, and signing identities.

Fixes #1874


Note

Medium Risk
Changes DM retry/idempotency semantics on a protocol path agents rely on; mitigated by focused forwarding, whitespace rejection, and cross-process proof tests, but incorrect key handling could still cause duplicate or blocked sends.

Overview
Fixes duplicate DMs when MCP send_dm retries outlive the process-local replay cache by forwarding the caller’s explicit idempotency_key through RelayAgentThinClient.dm to Relaycast, aligned with the existing relaycast client behavior. Unkeyed sends stay separate.

The MCP tool now trims idempotency_key before replay and upstream use and rejects whitespace-only keys, so a padded key cannot trim to empty and get a new random key per process (which would create a second message).

Coverage adds MCP protocol tests (replay boundaries, trim/reject, SDK option forwarding), SDK thin-client expectations, a startup regression on keyed retries, a RelayFlow fresh-process proof (1874-mcp-dm-idempotency), and a changelog note. Agent-workforce trajectory compaction files record the review/fix narrative but are not runtime code.

Reviewed by Cursor Bugbot for commit 040e655. Bugbot is set up for automated code reviews on this repo. Configure here.

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.
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: da9f4abb-b678-417f-bdd1-6e482ac59e16

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

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 <noreply@anthropic.com>
@khaliqgant

Copy link
Copy Markdown
Member

Superseded by #1882, which includes the explicit idempotency-key forwarding from this PR and also fixes the Bun standalone's duplicate MCP server startup, covers unkeyed sends, and adds a compiled-binary regression test.

@khaliqgant khaliqgant closed this Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP send_dm creates duplicate messages because idempotency_key stops at the process boundary

1 participant