Keep the mail broker from orphaning a reply to an unknown parent

[teamleaderleo]

agent-chat/mail/core.ts has two places where append() disagrees with the contract the file documents for itself. Both are pure unit-level defects in the broker, found while reading the module for the agent-mail RFC (manaflow-ai/cmuxterm-hq#851).

A reply to an unknown parent silently became a new thread root

append() looked up inReplyTo and passed the parent's threadId as a fallback. When the parent was unknown the fallback was undefined, so normalizeEnvelope fell through to its last resort and used the message's own id as the thread. The result was an envelope stamped kind: "reply" with inReplyTo: <parent> and references: [<parent>] that was nonetheless the root of a second thread, so thread(parentId) never returned it and the conversation was split with no error anywhere.

reply() refuses the same input, and its doc comment says why it exists:

Keeping this operation on the broker prevents adapters from accidentally creating a new thread when they only have a parent message ID.

That is exactly what the primitive underneath it did. append() now raises the same error, before normalizeEnvelope, so nothing is committed.

The case that has to keep working is a reply forwarded from another broker, which has the thread but not the parent message. Those callers pass threadId explicitly, and normalizeEnvelope already prefers a declared threadId over the parent's, so the guard only fires when there is genuinely nothing to attach the reply to. The test pins this. Re-appending a full MailEnvelope also keeps working, since a normalized envelope always carries a threadId.

An idempotent retry faulted when it reordered the recipients

fingerprint() is the tamper check behind MailConflictError, and canonicalize() sorts object keys but preserves array order. Recipients are deduped into a Set and never sorted, and deliveries are keyed by address, so the field is set-valued everywhere except in the fingerprint. Retrying ["codex","claude"] after ["claude","codex"] therefore looked like someone had reused the id with different content, defeating the retry safety the same function goes out of its way to provide for a generated createdAt two lines below.

fingerprint() now sorts recipients for the comparison only. The stored envelope keeps the order the first append supplied, so the recipient list and the deliveries array are unchanged. references is deliberately left alone: its order is the ancestry chain. Adding or dropping a recipient still conflicts, which the test asserts.

Verification

Red then green on the same command, bun test ./test/mail.test.ts in agent-chat/:

Commit 1 (1da8a467767, tests only):

(fail) append refuses a reply to a parent it has never seen
  error: append should refuse a reply whose parent this broker has never seen
(fail) append stays idempotent when a retry reorders the recipients
  MailConflictError: mail id fanout was already appended with different content
 1 pass
 2 fail

Commit 2 (f1a6abd40d3, the fix):

 3 pass
 0 fail

Whole package, bun run-tests.ts: 35 pass, 0 fail across 8 files. bun x tsc --noEmit: clean. Both run in CI through the Run agent-chat unit tests step in .github/workflows/ci-guards.yml.

No behavior change for any current caller: agent-chat/test/mail.test.ts is still the only importer of this module, so there is no wired surface to regress and nothing user-visible to dogfood. The value is that the in-flight mail work does not inherit a broker that splits threads quietly.

Changelog

none

🤖 Generated with Claude Code

---

Summary by cubic

Fixes two defects where the mail broker's append() disagreed with its own contract: it could orphan a reply as a new thread root, and it could reject an idempotent retry that simply reordered recipients.

• append() now refuses a reply whose parent it has never seen, throwing an exported MailUnknownParentError (with a code and parent id) instead of silently rooting a second thread at the reply's own id; reply() raises the same typed error. The guard also rejects a prebuilt envelope whose thread is the reply's own id and treats a blank threadId as absent, closing the shapes createMail can produce, while an explicit thread name still goes through so replicating a reply from another broker keeps working.
• fingerprint() now sorts recipients for the comparison only, so a retry listing the same recipients in another order is an idempotent no-op instead of a MailConflictError. The stored envelope keeps the first append's order, references order is preserved, and changing the recipient set still conflicts.

Tests added for both cases; no behavior change for current callers since the test file remains the only importer of this module.

^{Written for commit c6220e5. Summary will update on new commits.}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

Warning

Review limit reached

Next included review available in 12 seconds.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 77af8083-b360-44d2-b2f1-d98777079d64

📥 Commits

Reviewing files that changed from the base of the PR and between 0c753fe and c6220e5.

📒 Files selected for processing (2)

• agent-chat/mail/core.ts
• agent-chat/test/mail.test.ts

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[teamleaderleo]

Review: a review subagent went through this at f1a6abd40d3. 7 hypotheses, 0 blocking, 2 should-fix, 3 nits, 2 refuted (the fingerprint change does not weaken conflict detection, and no other caller of append/reply/publish reaches the guard). Verdict was safe to merge as-is; both should-fix items are now fixed in c6220e58b01.

Fixed:

• The new failure was a bare Error whose message contained unknown mail message <id>, which is verbatim what updateDelivery throws for an unknown message id. A caller could not tell the two apart, and my own test matched that substring, so it could have passed for the wrong reason. There is now an exported MailUnknownParentError with a MAIL_UNKNOWN_PARENT code and the parent id, matching MailConflictError and MailFanoutError, the message no longer overlaps, and the test asserts on the class.
• The guard only covered an absent threadId, so createMail slipped past it. createMail normalizes with no broker to look the parent up in, roots the envelope at its own id, and hands append a threadId that looks explicit, so broker.append(createMail({ ...inReplyTo: "never-appended" })) still produced the exact orphan this PR is about. A thread is rooted at a message that is not itself a reply, so a reply naming its own id as its thread is that orphan rather than a federated one. append now refuses that shape.
• Nit MAIL-04 along with it: the escape hatch keyed on !== undefined, so threadId: "" bypassed the guard and named an empty thread. Blank is now treated as absent in the guard, and normalizeEnvelope falls back instead of storing a blank thread id. No caller passes a blank one today.

Left:

• MAIL-03, the refused-append postcondition at test/mail.test.ts:124 is under-asserted. broker.get(id) falsy plus list().length === 0 is what I have, and I extended the same pair to both new cases. Adding a deliveries-level assertion would say more, but nothing commits before the throw, so it would be asserting on a code path that does not exist.
• MAIL-05, error precedence: an input that is both malformed (no sender, or over maxRecipients) and an orphan reply now reports the unknown-parent error instead of the validation error, because the guard runs before normalizeEnvelope. Both are programmer errors on the same call and neither is caught by type, so I would rather keep the guard where it can still see the un-normalized input than reorder for the message.

Verification on c6220e58b01, from agent-chat/: bun test ./test/mail.test.ts 3 pass 0 fail, bun run-tests.ts 35 pass 0 fail across 8 files plus 20 scripts, bun x tsc --noEmit clean. Red check for the new createMail assertion, with only the new guard neutralized: (fail) append refuses a reply to a parent it has never seen, append should refuse a prebuilt reply whose thread is its own id. Restored to green in the same run.

[teamleaderleo]

Merging on green under the standing rule for fixes: review subagent done, findings posted above and addressed.

No fleet dogfood evidence for this one, and the reason is structural rather than me skipping it: PR #8029 disabled Vercel branch previews, so an unmerged change to the deployed services has no preview URL for an app build to talk to. A fleet build would exercise main, not this branch. The behavior is covered by the tests recorded above instead, and I have said on the PR which verification did not run.

[github-actions]

Merge receipt for c6220e58b0: every check was green at merge (10 verified; 13 skipped by policy). Full suite runs on main after merge.