diff --git a/specs/api/agent-inbox-v1.md b/specs/api/agent-inbox-v1.md index 25617fe..68e2ba2 100644 --- a/specs/api/agent-inbox-v1.md +++ b/specs/api/agent-inbox-v1.md @@ -21,7 +21,9 @@ inbox's "Mine" folder works without inventing users. Multi-Agent identity is a l increment, added when there is a second Agent. This document covers the whole v1 surface. **HT-17 implemented §3's read paths and the -conventions below; HT-18 implemented §4a–4b; HT-16 amended §4a with send idempotency.** +conventions below; HT-18 implemented §4a–4b; HT-16 amended §4a with send idempotency; HT-49 +amended §4a's `References` derivation to append the reply's own minted id (a provider — +Gmail, confirmed live — can rewrite `Message-ID` on send; threading.md §2a).** The v1.1 additions land per-ticket: HT-26 (status model), HT-27 (`preview` + `number`), HT-28 (notes), HT-29 (tags), HT-30 (delete), HT-31 (assignee), HT-32 (open tracking). @@ -198,11 +200,19 @@ from the conversation, so the client never sets recipients or threading headers: - **`subject`** = the conversation's `subject`, prefixed with `Re:` plus a space if it isn't already (case-insensitive check — never double-prefix to `Re: Re:`). - **`In-Reply-To`** = the `messageId` of the conversation's most-recent INBOUND thread (the - customer message being answered), if it has one; **`References`** = the `messageId`s of - all prior threads in chronological order that have one. These are for the customer's mail - client to thread the reply in THEIR inbox — Helpthread's own threading never depends on - them (it is outbound-token-anchored; threading.md §2). Omitted when no prior message-id - exists (e.g. an inbound message that arrived without a `Message-ID`). + customer message being answered), if it has one; omitted when no prior message-id exists + (e.g. an inbound message that arrived without a `Message-ID`). +- **`References`** = the `messageId`s of all prior threads in chronological order that have + one, followed by this reply's OWN freshly-minted `messageId` as the FINAL entry — appended + by `sendReply` itself (`src/mail/send.ts`), unconditionally, even when no prior thread has a + `messageId` at all (a first reply then gets a one-element `References: [messageId]`, never + omitted the way `In-Reply-To` can be). These are for the customer's mail client to thread + the reply in THEIR inbox — Helpthread's own threading never depends on them (it is + outbound-token-anchored; threading.md §2) — but the reply's own minted id riding in + `References` is now load-bearing in one specific way (HT-49, threading.md §2a): some + providers (Gmail, confirmed live) rewrite the wire `Message-ID` to their own generated id, + so `References` — which such providers do NOT rewrite — is the channel that actually gets + the signed token back into the customer's reply when that happens. The handler then calls `sendReply` (`src/mail/send.ts`), passing the `Idempotency-Key` value through. `sendReply` mints the reply token into the outbound `Message-ID` (on a genuinely @@ -387,6 +397,23 @@ above. ## 7. Changelog +- **v1.1 (2026-07-17, HT-49 review fix).** `InboxApiDeps.selfEchoGuard` (optional, absent + by default): when present — and when the sender reports a provider message id for a + resolvable outbound mailbox — the send path best-effort pre-seeds a successful reply's + own sent-message echo as suppressed in the inbound delivery ledger, so a transport that + reflects sent mail back into its own mailbox (Gmail, confirmed live) normally does not + re-ingest it as a phantom inbound message — a consequence of the `References` change + below now carrying a verifiable token into that self-echo too. Best-effort, not a + guarantee: reconcile can win the documented pre-seeding race and ingest that one echo + first (`inbound-ingestion.md` §5's HT-49 amendment, "Known residual"). See + `src/mail/send.ts`'s "The reply token's own self-echo" section for the full mechanism. + No other §4a behavior changed; a deployment that leaves this absent behaves exactly as + before. +- **v1.1 (2026-07-17, HT-49).** §4a's `References` derivation now appends the reply's own + freshly-minted `messageId` as the final entry, after the derived ancestor chain — fixing + live-observed thread splits where a provider (Gmail, confirmed) rewrites the outbound + wire `Message-ID`, discarding the token from its one prior channel. See + threading.md §2a and sending.md §4 for the full mechanism; no other §4a behavior changed. - **v1.1 (2026-07-16, HT-46).** `ThreadView.attachments`: inbound attachment metadata + a signed `BlobStore` URL, `[]` by default and config-gated (absent `attachments` deps at the composition root, §4's `InboxApiDeps`, same posture as open tracking) — a diff --git a/specs/mail/inbound-ingestion.md b/specs/mail/inbound-ingestion.md index b991984..4b60ddd 100644 --- a/specs/mail/inbound-ingestion.md +++ b/specs/mail/inbound-ingestion.md @@ -240,6 +240,50 @@ loop yet; the suppression that matters now is the verifiable own-message loop ru A suppressed message is recorded in the ledger (`suppressed`, with the reason) — visible, auditable, never a silent drop. +**HT-49 amendment: the `Message-ID` loop-suppression correlation is defeated for a +provider that rewrites it (Gmail, confirmed live) — a second, ledger-level guard closes +the resulting gap.** The rule above ("our exact outbound `Message-ID` ... appearing as +this message's `Message-ID`") assumes the provider transmits `Message-ID` unaltered end +to end. `specs/mail/sending.md`'s HT-49 amendment records live evidence that Gmail's +`users.messages.send` does not: it accepts the engine's verbatim `Message-ID` but +substitutes its own generated id on the wire. Concretely, this means: (1) every outbound +reply now also carries the reply token as the FINAL entry of its own `References` chain +(threading.md §2a) — a second, provider-durable channel for the SAME token — and (2) when +that reply's own sent copy is reflected back into the mailbox it was sent from (Gmail +delivers a sent message into the same mailbox; the reconcile pipeline, `src/mail/ +gmail-reconcile.ts`, ingests it like any other inbound message), `isOwnMessageReflection` +above never fires for it — the message's OWN `Message-ID` is Gmail's substitute, not our +token, so the one correlation this rule implements finds nothing. Without a further guard, +`decideThreading` would then find the token in `References` and `append` — the agent's own +sent reply stored a second time as a phantom `direction: 'inbound'` message in the very +conversation it belongs to. + +The closing guard is NOT an extension of the `Message-ID`/`References`/`In-Reply-To` +correlation above (deliberately — see this section's own warning against trusting +sender-controlled headers, and the `isOwnMessageReflection` doc comment's note that a +customer's own out-of-office reply legitimately carries our token in exactly the same +`References` position and must still be ingested). Instead, `src/mail/send.ts` — right +after a successful send whose sender reports a `providerMessageId` +(`EmailSendResult.providerMessageId`, the SAME id the transport later reports for that +exact message during reconcile) — pre-seeds `(mailboxId, providerMessageId)` as an +ALREADY-`suppressed` row in the delivery ledger itself +(`InboundDeliveryStore.preSuppressOwnSend`, §4's idempotency/claim machinery, unchanged). +When reconcile later lists that same provider id, `claim()`'s ordinary "terminal row, do +not double-process" branch (§4) absorbs it — no new suppression code path, no change to +`decideThreading`, no heuristic on message content. This is a ledger-level, `providerMessageId`-keyed +correlation — a DIFFERENT (and more precise) mechanism than the `Message-ID` correlation +this section otherwise describes, chosen precisely because it does not touch the +customer-autoresponder case above at all. + +**Known residual: a race, conceded rather than corrected.** The pre-seed happens +AFTER the send resolves; if reconcile's own `claim()` for the same provider id wins that +race first (an unusually fast push-triggered reconcile), the message ingests normally +before the pre-seed ever runs — `preSuppressOwnSend` then finds the key already claimed +and is a no-op (it never overwrites an existing row, `src/store/inbound-deliveries.ts`'s +doc comment). This reproduces the PRE-HT-49-fix failure mode for that one send, not a new +one, and is not silently hidden: the phantom message is still recorded and visible in the +conversation, exactly as it would have been before this guard existed. + ## 6. Observability and the forged-token signal Each ingest emits a structured record: `mailboxId`, `providerMessageId`, the transport diff --git a/specs/mail/sending.md b/specs/mail/sending.md index e4d48ab..de46d0b 100644 --- a/specs/mail/sending.md +++ b/specs/mail/sending.md @@ -1,6 +1,6 @@ # Outbound sending & the reply-token lifecycle -Status: accepted (HT-15, HT-16). Companion to [threading.md](./threading.md) — that +Status: accepted (HT-15, HT-16, HT-49). Companion to [threading.md](./threading.md) — that spec decides which conversation an *inbound* message joins; this one covers how an *outbound* reply is minted, persisted, and sent, and is where the threading model's authority actually originates. @@ -174,6 +174,50 @@ accept raw MIME; reject any that will not carry `Message-ID` unaltered. The in-repo fake used by the engine tests proves only that `sendReply` *passes* the value to the seam — not that any given adapter preserves it on the wire. +**A compliant adapter is not sufficient — the provider's OWN infrastructure can +still rewrite `Message-ID` after transmission (HT-49, live production evidence, +2026-07-17).** Gmail's `users.messages.send` accepted the Gmail adapter's +verbatim `Message-ID` on the request and substituted its own generated id on +the wire — a rewrite downstream of transmission, outside the adapter's control, +and not a violation of the contract above (the Gmail adapter's own wire-level +contract test still passes: it proves what it sends, not what Gmail's server +does with it afterward). See threading.md §2a for the full story and the fix: +`sendReply` (`src/mail/send.ts`) now ALSO places its own minted `messageId` as +the final entry of that same reply's `References` header — a channel Gmail +does not rewrite — so the token survives even when `Message-ID` itself does +not. The adapter contract for `References` is: every atom transmitted is +transmitted verbatim and in its given order — never rewritten, reordered, or +substituted. Unlike `Message-ID`, though, the ancestor portion of `References` +carries attacker-influenced inbound msg-ids, so an adapter MAY sanitize by +DROPPING an individual unsafe atom (header-injection / oversize defense — the +Gmail adapter's `isSafeMsgId` filter, `src/providers/adapters/gmail/mime.ts`, +does exactly this rather than letting one crafted stored ancestor id block +every future reply to its conversation). The engine-minted final entry passes +any such filter by construction (`reply-token.ts`'s bounded `[A-Za-z0-9_-]` / +`.` / `@` charset contains no control characters and stays far under the +octet bound) and MUST reach the wire intact — an adapter that drops or alters +IT is as unusable as one that rewrites `Message-ID`. The HT-49 fix is in what +the engine puts into `References` before handing it to the adapter, not a +change to this adapter contract. + +**Review-fix amendment: a self-reflecting transport requires ALSO suppressing the sent +message's own echo (`src/mail/send.ts`'s "The reply token's own self-echo" section).** +Placing the reply token in `References` unconditionally has a consequence the paragraph +above does not by itself address: Gmail (confirmed live) delivers the SENT message back +into the mailbox it was sent from, and that self-echo now carries a verifiable token — +one `inbound-ingestion.md` §5's `Message-ID`-only loop guard cannot recognize, for the +exact same reason as above (Gmail rewrites the echo's `Message-ID` too). Left alone, the +echo would `append` into its own conversation as a phantom inbound message. `sendReply` +closes this immediately after a successful send: if `EmailSender.send()` returned an +`EmailSendResult.providerMessageId` (`src/providers/email-sender.ts`) — the SAME id the +transport later reports for that message during reconcile — it resolves `SendReplyInput. +from` to its `MailboxRecord` and pre-seeds `(mailboxId, providerMessageId)` as an +already-`suppressed` row in the inbound delivery ledger (`InboundDeliveryStore. +preSuppressOwnSend`; `inbound-ingestion.md` §5's HT-49 amendment has the full mechanism +and its one known residual race). This is OPTIONAL (`SendReplyDeps.selfEchoGuard`) and a +no-op wherever absent or wherever the sender reports no `providerMessageId` — a deployment +with no self-reflecting transport configured behaves exactly as before this guard existed. + **Recommended: a provider SHOULD de-duplicate on `Message-ID` (HT-16).** This is not a precondition the engine requires — at-least-once delivery (§3a) holds with or without it — but it is not an aside either: it is the one thing @@ -213,10 +257,14 @@ Deliberately narrow; each deferral below has a named later home: directly (e.g. from a test or a manual trigger), never on a timer. - **Reply to an existing conversation only.** Agent-*initiated* brand-new conversations are a separate later flow. -- **`In-Reply-To`/`References` are caller-supplied** (from the inbound message - being answered). Deriving the full `References` chain from stored threads is a - later refinement. Once persisted into `send_envelope` (§3a) that snapshot is - authoritative for every retry regardless of how it was originally derived. +- **`In-Reply-To`/`References` are caller-supplied ANCESTOR ids** (from the + inbound message being answered; `agent-inbox-v1.md` §4a's `deriveReplyHeaders` + derives them from stored threads today). `sendReply` then APPENDS its own + freshly-minted `messageId` as the final `References` entry unconditionally + (HT-49; threading.md §2a) — the caller-supplied field is never itself the + reply's own id. Once persisted into `send_envelope` (§3a) that full chain, + own id included, is authoritative for every retry regardless of how the + ancestor portion was originally derived. - **A missing or deleted conversation is refused** — the token is minted first (before `appendThread` resolves) and then discarded on refusal; only persistence and sending are skipped, and the sender is never called (mirrors diff --git a/specs/mail/threading.md b/specs/mail/threading.md index 5ac6cd0..acbdb28 100644 --- a/specs/mail/threading.md +++ b/specs/mail/threading.md @@ -25,6 +25,18 @@ The properties that ARE the spec, independent of encoding: **Contrast with the observed reference format.** The fixtures show a reference helpdesk emitting Message-IDs shaped like `` — e.g. `` (reply-with-reference.json, `agentReplyEmail.messageId`; the token value in the committed fixtures is a redacted placeholder — the real capability token is never published). Notably `{threadId}` there is a *thread* id (36), not the conversation id (15) — conversation is resolved via the thread's parent, not encoded directly. This is cited only as evidence the "signed token in the outbound Message-ID" pattern works in production (charter §2); Helpthread's `sig` derivation, secret, and truncation are unrelated to whatever that system does internally, which was never observed. +## 2a. The token also rides in `References` (HT-49 — a provider can rewrite `Message-ID`) + +**RESOLVED, live production evidence, 2026-07-17 (first HT-44 run against real Gmail).** §2 describes the token embedded in the outbound `Message-ID`; that remains true and unchanged. But `Message-ID` is not guaranteed to survive transmission unaltered: Gmail's `users.messages.send` accepted the engine's verbatim-set `Message-ID` on the request and REPLACED it on the wire with a Gmail-generated id (`` — confirmed from the raw copy Gmail itself returned on reconcile of the sent message's self-echo). This is not a violation of the `EmailSender` contract (specs/mail/sending.md §4, `src/providers/email-sender.ts`) — the adapter transmitted `Message-ID` verbatim as required; the rewrite happens server-side, downstream of transmission, outside any adapter's control. Its effect on threading is the same either way: the customer's reply carried `In-Reply-To`/its trailing `References` entry pointing at Gmail's substituted id, with our token nowhere on the wire — `decideThreading` correctly found no verified token and (§3 rule 4, invariant #5) started a NEW conversation instead of appending. Tonight's failure is preserved as a fixture reproducing it exactly: `src/mail/ingest.test.ts`'s "the exact live-production failure" test. + +**The fix: the outbound reply's own minted `messageId` ALSO rides as the FINAL entry of that reply's own `References` header** (`src/mail/send.ts`, `sendReply`), appended after any ancestor ids — unconditionally, even on a first reply with no ancestors (a one-element `References: [messageId]`). `References`, unlike `Message-ID`, is not rewritten by Gmail. An RFC-5322-compliant reply's own `References` is built as `{original References} + {original Message-ID}` (§3.6.4) — so when the customer replies, their client's `References` becomes `[...ourOutboundReferences, gmailRewrittenId]`, i.e. `[...ancestors, ourMintedToken, gmailRewrittenId]`. The token lands ONE POSITION BEFORE the trailing foreign id — never last, never in `In-Reply-To` (which still correctly names the specific ancestor message being answered, not this reply's own id — left unchanged by this fix). + +**Zero threading-decision code changed.** §3's algorithm already scans `References` newest-first (`src/mail/thread.ts`'s `buildCandidates`, reversing wire order before scanning) — exactly what is needed to skip the foreign trailing id and find our token immediately behind it. This section moves *where the token rides on the outbound side*; it does not touch how an inbound decision is made, does not add a heuristic, and does not weaken "no verified token ⇒ new conversation." Verified, not assumed: `src/mail/thread.ts` is unmodified by HT-49, and the fixture above passes through the existing scan unchanged. + +**Every future outbound reply therefore carries the token TWICE** — once in `Message-ID` (§2, the primary channel, un-rewritten by providers that respect the `EmailSender` contract) and once as the final `References` entry (§2a, the provider-durable backup channel). Either surviving to the customer's reply is sufficient for `decideThreading` to append correctly; both surviving is redundant, not conflicting (the newest-first scan tries `In-Reply-To` first, so an un-rewritten `Message-ID`/`In-Reply-To` pair is still found first when it survives). + +**Review-fix amendment: this section's own reply also reflects back into its own mailbox — a SEPARATE guard, not this section's scan, closes it.** Putting the token in `References` unconditionally means the SENT message's own self-echo (Gmail delivers a sent message into the mailbox it was sent from; `src/mail/gmail-reconcile.ts` ingests it like any other inbound message) now ALSO carries a verifiable token in `References` — and `inbound-ingestion.md` §5's `Message-ID`-based loop guard (`isOwnMessageReflection`) cannot catch it, for the exact reason this section exists: Gmail rewrites the self-echo's `Message-ID` too. Left unguarded, that self-echo would `append` into its own conversation as a phantom inbound message. This is NOT fixed by touching `decideThreading` or this section's scan (doing so would also misfire on a customer's legitimate autoresponder reply, which carries our token in `References` the exact same way) — it is fixed one layer earlier, in the delivery ledger itself, before `decideThreading` ever runs: see `inbound-ingestion.md` §5's "HT-49 amendment" for the full mechanism (`src/mail/send.ts`'s `preSuppressOwnSend`) and its one known residual race. + ## 3. Inbound threading decision — the algorithm Ordered, testable procedure applied to every inbound message: diff --git a/src/api/conversations.ts b/src/api/conversations.ts index 41ab28d..b619e4a 100644 --- a/src/api/conversations.ts +++ b/src/api/conversations.ts @@ -16,7 +16,7 @@ */ import type { Keyring } from '../mail/reply-token.js' -import { sendReply } from '../mail/send.js' +import { type SelfEchoGuardDeps, sendReply } from '../mail/send.js' import type { BlobStore, EmailSender } from '../providers/index.js' import type { StoredThreadAttachment, ThreadAttachmentStore } from '../store/attachments.js' import { @@ -371,6 +371,7 @@ export async function handleReply( mailDomain: string supportAddress: string openTracking?: { publicBaseUrl: string } + selfEchoGuard?: SelfEchoGuardDeps }, ): Promise { if (!isUuid(id)) { @@ -440,6 +441,7 @@ export async function handleReply( keyring: deps.keyring, mailDomain: deps.mailDomain, ...(deps.openTracking !== undefined ? { openTracking: deps.openTracking } : {}), + ...(deps.selfEchoGuard !== undefined ? { selfEchoGuard: deps.selfEchoGuard } : {}), }, ) diff --git a/src/api/index.test.ts b/src/api/index.test.ts index df0d761..c27ae19 100644 --- a/src/api/index.test.ts +++ b/src/api/index.test.ts @@ -634,13 +634,6 @@ describe('createInboxApi', () => { }) expect(sent).toHaveLength(1) - expect(sent[0]).toMatchObject({ - to: ['customer@example.test'], - from: SUPPORT_ADDRESS, - subject: 'Re: Help with my order', - inReplyTo: '', - references: [''], - }) const updated = await store.getConversation(conversationId, { includeDeleted: false }) const outboundThread = updated?.threads.find((t) => t.id === body.id) @@ -649,6 +642,18 @@ describe('createInboxApi', () => { expect(outboundThread?.deliveryStatus).toBe('sent') // The engine-minted Message-ID is transmitted verbatim (providers/email-sender.ts's contract). expect(sent[0].messageId).toBe(outboundThread?.messageId) + + expect(sent[0]).toMatchObject({ + to: ['customer@example.test'], + from: SUPPORT_ADDRESS, + subject: 'Re: Help with my order', + inReplyTo: '', + // HT-49: References carries the reply's OWN minted messageId as its + // FINAL entry (after the derived ancestor chain) — the durable + // channel for the reply token once a provider (Gmail) rewrites + // Message-ID on send. See send.ts's module doc. + references: ['', outboundThread?.messageId], + }) }) it('sent-but-mark-sent-fails still returns 201, not 502 (the message WAS delivered — never prompt a resend)', async () => { diff --git a/src/api/index.ts b/src/api/index.ts index 8147efa..aa73c23 100644 --- a/src/api/index.ts +++ b/src/api/index.ts @@ -31,6 +31,7 @@ import { TRANSPARENT_GIF, verifyViewToken } from '../mail/open-tracking.js' import type { Keyring } from '../mail/reply-token.js' +import type { SelfEchoGuardDeps } from '../mail/send.js' import type { BlobStore, EmailSender } from '../providers/index.js' import type { ThreadAttachmentStore } from '../store/attachments.js' import type { ConversationStore } from '../store/conversations.js' @@ -147,6 +148,24 @@ export interface InboxApiDeps { * posture above. */ attachments?: { store: ThreadAttachmentStore; blobStore: BlobStore } + /** + * The self-echo guard `sendReply` accepts (HT-49 review fix; `src/mail/ + * send.ts`'s `SelfEchoGuardDeps`): ABSENT BY DEFAULT — a deployment with no + * self-reflecting transport configured (no Gmail mailbox connected) simply + * never sets this, and reply-sending behaves exactly as before this guard + * existed. When present, a successful reply's own sent-message echo is + * best-effort pre-suppressed in the inbound delivery ledger so a transport + * that delivers sent mail back into its own mailbox (Gmail, confirmed + * live) normally does not re-ingest it as a phantom inbound message. + * Best-effort, not a guarantee: the pre-seed runs only AFTER the provider + * send succeeds, so an unusually fast reconcile can claim `(mailboxId, + * providerMessageId)` first and ingest that one echo before the pre-seed + * lands — reproducing the pre-guard failure mode (a visible phantom + * inbound message in that conversation) for that single send, never a new + * one. See `inbound-ingestion.md` §5's HT-49 amendment ("Known residual") + * for the conceded race. + */ + selfEchoGuard?: SelfEchoGuardDeps } /** @@ -310,6 +329,7 @@ export function createInboxApi(deps: InboxApiDeps): (request: Request) => Promis mailDomain: deps.mailDomain, supportAddress: deps.supportAddress, ...(deps.openTracking !== undefined ? { openTracking: deps.openTracking } : {}), + ...(deps.selfEchoGuard !== undefined ? { selfEchoGuard: deps.selfEchoGuard } : {}), }) case 'gmail-connect': diff --git a/src/composition/root.ts b/src/composition/root.ts index 0643f37..f1d65cf 100644 --- a/src/composition/root.ts +++ b/src/composition/root.ts @@ -232,6 +232,12 @@ export async function buildApp( gmailConnect, gmailDisconnect, attachments: { store: attachmentStore, blobStore }, + // HT-49 review fix: Gmail delivers a sent reply's own copy back into the + // SAME mailbox it was sent from, where reconcile would otherwise re-ingest + // it as a phantom inbound message (src/mail/send.ts's "The reply token's + // own self-echo" section). Wired unconditionally here — every deployment + // this root builds is Gmail-backed. + selfEchoGuard: { mailboxStore, inboundDeliveryStore }, }) // --- The reconcile handler the queue drain dispatches to. --- diff --git a/src/mail/delivery-worker.ts b/src/mail/delivery-worker.ts index 4f7ce04..a44fb6b 100644 --- a/src/mail/delivery-worker.ts +++ b/src/mail/delivery-worker.ts @@ -44,6 +44,7 @@ import { assertLeaseExceedsSenderBound, attemptDeliveryOfClaimedThread, DEFAULT_LEASE_MS, + type SelfEchoGuardDeps, } from './send.js' /** Default age a `'pending'` row must reach before this worker considers it stuck rather than merely in flight. */ @@ -56,6 +57,14 @@ const DEFAULT_BATCH_SIZE = 50 export interface DeliveryWorkerDeps { store: ConversationStore sender: EmailSender + /** + * The same self-echo guard `sendReply` accepts (`./send.js`'s + * `SelfEchoGuardDeps`, HT-49 review fix) — a retried send through THIS + * worker's `attemptDeliveryOfClaimedThread` call is just as capable of + * producing a self-echo as `sendReply`'s own retry path, so it needs the + * same pre-suppression. ABSENT BY DEFAULT, a no-op when unset. + */ + selfEchoGuard?: SelfEchoGuardDeps } /** Tuning knobs for one sweep; every field defaults, so `runDeliveryWorker(deps)` alone is a complete, reasonable call. */ @@ -121,6 +130,7 @@ export async function runDeliveryWorker( const result = await attemptDeliveryOfClaimedThread(claimed, { store: deps.store, sender: deps.sender, + selfEchoGuard: deps.selfEchoGuard, }) if (result.ok) { sent++ diff --git a/src/mail/ingest.test.ts b/src/mail/ingest.test.ts index 0080cb0..b2d83ef 100644 --- a/src/mail/ingest.test.ts +++ b/src/mail/ingest.test.ts @@ -332,6 +332,148 @@ describe('ingestInboundMessage', () => { expect(await countRows(db, 'threads')).toBe(2) }) + // --- HT-49: the exact live-production failure, reproduced as a fixture --- + // + // Live evidence (2026-07-17, first HT-44 run against real Gmail): Gmail's + // `users.messages.send` REPLACED the engine-minted Message-ID with a + // Gmail-generated one on the wire. The customer's reply therefore carried + // `In-Reply-To`/a trailing `References` entry naming Gmail's FOREIGN id — + // our token was nowhere to be found by a scan that only checked those — + // and `decideThreading` (correctly, per invariant #5: no verified token ⇒ + // new conversation) forked a new conversation instead of appending. + // + // The fix (`src/mail/send.ts`): the outbound reply's own minted messageId + // rides as the FINAL entry of the OUTBOUND References chain, which Gmail + // does NOT rewrite. An RFC-5322-compliant reply's own References becomes + // `{our outbound References} + {gmail's rewritten id}` — so the customer's + // reply carries our token ONE POSITION BEFORE the trailing foreign id, + // never in In-Reply-To, never last. `decideThreading`'s existing + // newest-first scan (`src/mail/thread.ts`, unmodified by this fix) skips + // the foreign trailing id and finds our token immediately behind it. + it('HT-49: a reply whose In-Reply-To is a FOREIGN (Gmail-rewritten) id and whose References carries our token mid-chain still threads into the original conversation', async () => { + const { db, deps, mailboxId } = await freshDeps() + + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + + // The outbound reply this engine sent: its own minted token, which + // `send.ts` placed as the FINAL References entry of ITS outbound mail. + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + // Gmail's server-side substitute for the wire Message-ID of that SAME + // outbound reply (the id the customer's mail client actually observed as + // the message's Message-ID, and therefore what its own In-Reply-To/ + // trailing References entry names) — never one of our tokens. + const gmailRewrittenId = '' + + const replyRaw = rawMessage( + { + From: 'customer@example.test', + To: 'support@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': '', + // Foreign — NOT our token. A scan that only checked In-Reply-To + // would find nothing and (wrongly) start a new conversation. + 'In-Reply-To': gmailRewrittenId, + // Our token rides mid-chain: ancestor, then our token, then the + // foreign id the customer's client appended last. + References: ` ${replyToken} ${gmailRewrittenId}`, + }, + 'Still broken, please help.', + ) + + const second = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-2', replyRaw), + deps, + ) + + expect(second).toMatchObject({ kind: 'stored', conversationId: first.conversationId }) + expect(await countRows(db, 'conversations')).toBe(1) + expect(await countRows(db, 'threads')).toBe(2) + }) + + // --- HT-49 review fix: the OUTBOUND reply's own self-echo, ingested by the + // SAME mailbox it was sent from, must not be re-appended as a phantom + // inbound message. ---------------------------------------------------------- + // + // Without the self-echo guard (`src/mail/send.ts`'s `SelfEchoGuardDeps`), + // this is EXACTLY the fixture above minus the customer ever replying: the + // self-echo carries our own token as its final References entry — a + // provider that rewrites Message-ID (Gmail, live-confirmed) means the loop + // guard (`isOwnMessageReflection`, which only checks the message's OWN + // Message-ID) never fires — so decideThreading finds the token and + // `append`s the agent's own sent reply into its own conversation a SECOND + // time, as a phantom `direction: 'inbound'` message whose `fromAddress` is + // the mailbox's own support address. This test proves that once + // `send.ts` has pre-seeded `(mailboxId, providerMessageId)` as suppressed + // (`InboundDeliveryStore.preSuppressOwnSend`), reconcile's later `ingest` + // call for that SAME provider id is suppressed instead — never appended. + it("HT-49 review fix: a self-echo of the agent's own sent reply — From the mailbox's OWN address, a foreign (Gmail-rewritten) Message-ID, our token as the FINAL References entry — is suppressed, not appended, once send.ts has pre-seeded its providerMessageId", async () => { + const { db, deps, mailboxId } = await freshDeps() + + const first = await ingestInboundMessage( + inboundDelivery(mailboxId, 'provider-msg-1', freshCustomerRaw()), + deps, + ) + if (first.kind !== 'stored') throw new Error('unreachable') + + // The agent's own reply: send.ts mints this token and places it as the + // FINAL References entry of the outbound mail (module doc, threading.md + // §2a) — this is what a raw self-echo of that SAME reply would carry. + const replyToken = mintReplyMessageId( + { conversationId: first.conversationId, threadId: 'outbound-t1', mailDomain: MAIL_DOMAIN }, + keyring, + ) + // Gmail's own substitute for the wire Message-ID of that outbound + // send — never one of our tokens (live-confirmed, HT-49). + const gmailRewrittenId = '' + // The SAME id Gmail's users.messages.send returned as EmailSendResult. + // providerMessageId for that send (src/providers/adapters/gmail/ + // sender.ts) — and the id gmail-reconcile.ts's history.list later + // reports for the self-echo. send.ts's self-echo guard pre-seeds + // EXACTLY this key before this ever reaches ingest. + const selfEchoProviderMessageId = 'gmail-self-echo-msg-id' + + // --- The pre-seed send.ts's selfEchoGuard performs right after the send + // succeeds (src/mail/send.ts's suppressSelfEcho). ----------------------- + await deps.inboundDeliveryStore.preSuppressOwnSend( + mailboxId, + selfEchoProviderMessageId, + 'own-outbound-self-echo', + ) + + // --- The self-echo itself: From is the MAILBOX'S OWN address (never a + // customer), Message-ID is Gmail's foreign substitute, and References + // ends with our own token — exactly what Gmail delivers back into the + // mailbox for the agent's own sent reply. ------------------------------ + const selfEchoRaw = rawMessage( + { + From: 'support@example.test', + To: 'customer@example.test', + Subject: 'Re: Help with my order', + 'Message-ID': gmailRewrittenId, + References: ` ${replyToken}`, + }, + "We're looking into it!", + ) + + const echoOutcome = await ingestInboundMessage( + inboundDelivery(mailboxId, selfEchoProviderMessageId, selfEchoRaw), + deps, + ) + + expect(echoOutcome).toMatchObject({ kind: 'suppressed' }) + // No second thread, no reopened/duplicated conversation — the pre-seeded + // ledger row absorbed the echo before decideThreading ever ran on it. + expect(await countRows(db, 'conversations')).toBe(1) + expect(await countRows(db, 'threads')).toBe(1) + }) + // --- spec §8: re-delivery of the same key → a no-op. ---------------------- it('re-delivery of the same (mailboxId, providerMessageId) is a no-op: one conversation, one thread, one stored ledger row', async () => { diff --git a/src/mail/send.test.ts b/src/mail/send.test.ts index 5ffa402..b67a406 100644 --- a/src/mail/send.test.ts +++ b/src/mail/send.test.ts @@ -3,6 +3,8 @@ import { createPgliteDb, type Db } from '../db/client.js' import { migrate } from '../db/migrate.js' import type { EmailSender, OutboundEmail } from '../providers/index.js' import { type ConversationStore, createConversationStore } from '../store/conversations.js' +import { createInboundDeliveryStore } from '../store/inbound-deliveries.js' +import { createMailboxStore } from '../store/mailboxes.js' import { verifyViewToken } from './open-tracking.js' import type { ParsedEmail } from './parse.js' import type { Keyring, SigningKey } from './reply-token.js' @@ -160,6 +162,123 @@ describe('sendReply', () => { }) }) + // HT-49: live production evidence (2026-07-17) showed Gmail's + // `users.messages.send` REPLACING the engine-minted Message-ID with a + // Gmail-generated one on the wire — so a customer's reply threading purely + // on `In-Reply-To`/the trailing `References` entry finds no verified token + // and (correctly, per invariant #5) starts a NEW conversation instead of + // appending. The fix: `sendReply` appends its own minted messageId as the + // FINAL References entry (module doc), which survives because Gmail does + // NOT rewrite References — so it rides along into the customer's reply + // one position before whatever foreign id the provider substituted. + it("HT-49: derived envelope — sendEnvelope.references ends with this reply's OWN minted messageId, after any ancestor ids", async () => { + const { store } = await freshStore() + const { conversationId } = await seedConversation(store) + const sender = fakeSender() + const deps: SendReplyDeps = { store, sender, keyring, mailDomain } + + const result = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + inReplyTo: '', + references: [''], + }, + deps, + ) + if (!result.ok) throw new Error('unreachable') + + // Sent on the wire (via the provider seam) — ends with our own token. + expect(sender.sent).toHaveLength(1) + expect(sender.sent[0].references).toEqual([ + '', + result.messageId, + ]) + // In-Reply-To is UNCHANGED — it still names the ancestor being answered, + // never this reply's own id. + expect(sender.sent[0].inReplyTo).toBe('') + + // Persisted verbatim in the outbound thread's send_envelope snapshot — + // what any later retry (a keyed replay, or the delivery worker) resends. + const conversation = await store.getConversation(conversationId) + const outbound = conversation?.threads.find((t) => t.id === result.threadId) + expect(outbound?.sendEnvelope?.references).toEqual([ + '', + result.messageId, + ]) + }) + + it('HT-49: a first reply with NO ancestor references still gets a one-element References: [ownMessageId]', async () => { + const { store } = await freshStore() + const { conversationId } = await seedConversation(store) + const sender = fakeSender() + const deps: SendReplyDeps = { store, sender, keyring, mailDomain } + + const result = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + }, + deps, + ) + if (!result.ok) throw new Error('unreachable') + + expect(sender.sent[0].references).toEqual([result.messageId]) + }) + + it('HT-49: the exact live-production failure — a customer reply whose In-Reply-To is a FOREIGN (Gmail-rewritten) id and whose References carries our token one position before it still threads into the original conversation', async () => { + const { store } = await freshStore() + const { conversationId } = await seedConversation(store) + const sender = fakeSender() + const deps: SendReplyDeps = { store, sender, keyring, mailDomain } + + const sent = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + inReplyTo: '', + references: [''], + }, + deps, + ) + if (!sent.ok) throw new Error('unreachable') + + // What the engine actually sent (mime.ts/gmail sender wire contract): + // References = [...ancestors, ourMintedToken]. + expect(sender.sent[0].references).toEqual(['', sent.messageId]) + + // Gmail REPLACES the wire Message-ID with its own id (live-confirmed + // 2026-07-17) — so the customer's mail client builds ITS OWN reply as + // In-Reply-To: {gmail's id}, References: {our outbound References} + + // {gmail's id}. Our token ends up MID-CHAIN, one position before the + // trailing foreign id — never last, never in In-Reply-To at all. + const gmailRewrittenId = '' + const customerReply = inboundReplyTo(gmailRewrittenId) + customerReply.references = [ + '', + sent.messageId, + gmailRewrittenId, + ] + + const decision = decideThreading(customerReply, keyring) + + expect(decision).toEqual({ + kind: 'append', + conversationId, + threadId: sent.threadId, + forgedTokenCount: 0, + }) + }) + it('send failure: returns { send-failed, persistedStatus: failed } and leaves the thread failed', async () => { const { store } = await freshStore() const { conversationId } = await seedConversation(store) @@ -301,6 +420,167 @@ describe('sendReply', () => { }) }) +// --- self-echo guard (HT-49 review fix) ------------------------------------- +// +// Gmail delivers a sent reply's own copy back into the SAME mailbox it was +// sent from; without this guard, the reply token this fix (HT-49) puts in +// EVERY outbound References would make that self-echo `append` into the very +// conversation it belongs to (module doc's "The reply token's own self-echo" +// section). These tests exercise `selfEchoGuard` directly against a real +// `InboundDeliveryStore`/`MailboxStore`, rather than a fake, so the +// suppressed row's shape (and `claim()`'s later behavior against it) is +// verified, not assumed. + +describe('sendReply self-echo guard (HT-49 review fix)', () => { + let db: Db | undefined + + afterEach(async () => { + await db?.close() + db = undefined + }) + + async function freshDeps() { + db = await createPgliteDb() + await migrate(db) + const store = createConversationStore(db) + const mailboxStore = createMailboxStore(db) + const inboundDeliveryStore = createInboundDeliveryStore(db) + await db.query("INSERT INTO mailboxes (address, provider) VALUES ($1, 'gmail')", [ + 'support@example.test', + ]) + const { conversationId } = await store.createConversation({ + subject: 'Help with my order', + customerEmail: 'customer@example.test', + firstMessage: { + direction: 'inbound', + messageId: '', + fromAddress: 'customer@example.test', + bodyText: 'Where is my order?', + }, + }) + return { db, store, mailboxStore, inboundDeliveryStore, conversationId } + } + + it("a successful send whose sender returns a providerMessageId pre-suppresses that id in the FROM mailbox's inbound delivery ledger", async () => { + const { store, mailboxStore, inboundDeliveryStore, conversationId } = await freshDeps() + const sender = fakeSender() // returns { providerMessageId: 'provider-1' } + const deps: SendReplyDeps = { + store, + sender, + keyring, + mailDomain, + selfEchoGuard: { mailboxStore, inboundDeliveryStore }, + } + + const result = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + }, + deps, + ) + if (!result.ok) throw new Error('unreachable') + + // The exact self-echo Gmail would later report via history.list for + // THIS send is already suppressed — claim() reports it as terminal, + // never as a fresh 'received' row ingest would append. + const mailbox = await mailboxStore.getMailboxByAddress('support@example.test') + const claim = await inboundDeliveryStore.claim(mailbox?.id ?? '', 'provider-1', 30_000) + expect(claim).toMatchObject({ + claimed: false, + delivery: { status: 'suppressed', lastError: 'own-outbound-self-echo' }, + }) + }) + + it('no selfEchoGuard configured: send succeeds exactly as before, no ledger row is created', async () => { + const { db, store, conversationId } = await freshDeps() + const sender = fakeSender() + const deps: SendReplyDeps = { store, sender, keyring, mailDomain } + + const result = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + }, + deps, + ) + + expect(result.ok).toBe(true) + // No mailbox lookup or ledger write ever happens without a configured guard. + const rows = await db.query('SELECT id FROM inbound_deliveries') + expect(rows).toHaveLength(0) + }) + + it('sender returns no providerMessageId: guard is configured but a no-op (nothing to correlate against)', async () => { + const { store, mailboxStore, inboundDeliveryStore, conversationId } = await freshDeps() + const sender: EmailSender = { + maxSendMs: 30_000, + async send() { + return {} // no providerMessageId + }, + } + const deps: SendReplyDeps = { + store, + sender, + keyring, + mailDomain, + selfEchoGuard: { mailboxStore, inboundDeliveryStore }, + } + + const result = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + }, + deps, + ) + + expect(result.ok).toBe(true) + const mailbox = await mailboxStore.getMailboxByAddress('support@example.test') + expect(mailbox).not.toBeNull() + // Nothing to correlate against without a providerMessageId — no row was seeded. + const claim = await inboundDeliveryStore.claim(mailbox?.id ?? '', 'whatever', 30_000) + expect(claim.claimed).toBe(true) // fresh — proves nothing was pre-seeded. + }) + + it('a failed send never pre-suppresses anything (nothing was delivered, so there is no self-echo to guard against)', async () => { + const { store, mailboxStore, inboundDeliveryStore, conversationId } = await freshDeps() + const sender = failingSender() + const deps: SendReplyDeps = { + store, + sender, + keyring, + mailDomain, + selfEchoGuard: { mailboxStore, inboundDeliveryStore }, + } + + const result = await sendReply( + { + conversationId, + from: 'support@example.test', + to: ['customer@example.test'], + subject: 'Re: Help with my order', + text: "We're looking into it!", + }, + deps, + ) + + expect(result.ok).toBe(false) + const mailbox = await mailboxStore.getMailboxByAddress('support@example.test') + const claim = await inboundDeliveryStore.claim(mailbox?.id ?? '', 'provider-1', 30_000) + expect(claim.claimed).toBe(true) // fresh — nothing was pre-seeded for a rejected send. + }) +}) + // --- idempotency (HT-16) ----------------------------------------------------- describe('sendReply idempotency (HT-16)', () => { @@ -451,7 +731,11 @@ describe('sendReply idempotency (HT-16)', () => { messageId: first.messageId, to: ['customer@example.test'], subject: 'Re: Help with my order', - references: [''], + // HT-49: References carries the reply's OWN minted messageId as its + // FINAL entry (after the persisted ancestor chain) — see send.ts's + // module doc. This is the ORIGINAL attempt's messageId/references, + // unaffected by the retry's different (ignored) references input. + references: ['', first.messageId], }) const conversation = await store.getConversation(conversationId) diff --git a/src/mail/send.ts b/src/mail/send.ts index 1684a1a..a951897 100644 --- a/src/mail/send.ts +++ b/src/mail/send.ts @@ -100,11 +100,82 @@ * emits canonical ids and callers pass those straight through, so this holds * by construction; it is called out because the token carries the string, not * a parsed UUID. + * + * ## References carries the reply token, not just Message-ID (HT-49) + * + * Live production evidence (2026-07-17, first HT-44 run against real Gmail): + * Gmail's `users.messages.send` accepted our verbatim-set `Message-ID` on the + * request but REPLACED it on the wire with a Gmail-generated id + * (``) — confirmed from the raw copy Gmail itself + * returned on reconcile of the sent message's self-echo. Every + * `EmailSender` adapter is still required to transmit `OutboundEmail.messageId` + * verbatim (`src/providers/email-sender.ts`'s module doc) — this is a + * provider-side rewrite downstream of that verbatim transmission, not a + * violation of it, and no adapter change closes it. The customer's reply + * therefore carried `In-Reply-To`/`References` pointing at GMAIL's id, with + * our minted token nowhere on the wire — `decideThreading` correctly found no + * verified token and (per invariant #5) started a NEW conversation instead of + * appending, splitting the thread. + * + * `References`, unlike `Message-ID`, is NOT rewritten by Gmail — and an + * RFC-5322-compliant reply's own `References` is built as + * `{original References} + {original Message-ID}` (§3.6.4). So this function + * appends its own freshly-minted `messageId` as the FINAL entry of the + * outbound `References` chain, after any ancestor ids — giving the token a + * second, provider-durable channel out onto the wire. When the customer + * replies, their client's own References becomes + * `[...ourReferences, gmailRewrittenId]` — i.e. + * `[...ancestors, ourMintedToken, gmailRewrittenId]` — and `decideThreading`'s + * existing newest-first scan (`src/mail/thread.ts`, `buildCandidates`) skips + * the foreign trailing id (no token, not ours to judge) and finds our token + * immediately behind it. `In-Reply-To` is left untouched: it still names the + * specific ancestor message being answered, not this reply's own id — see + * `specs/mail/threading.md` §2a for the full spec of this fix, and + * `specs/mail/sending.md`/`specs/api/agent-inbox-v1.md` §4a for the + * corresponding header-derivation wording. Zero threading-decision code + * changed: verified, not assumed — `src/mail/thread.ts` is untouched by this + * fix, and a fixture reproducing tonight's exact failure (`src/mail/ + * ingest.test.ts`) threads correctly through the existing scan unmodified. + * + * ## The reply token's own self-echo, and how it is suppressed (HT-49 review fix) + * + * Putting a verifiable token in EVERY outbound reply's `References` (above) + * has a sharp edge: some transports (Gmail, confirmed live) deliver the SENT + * message back into the very mailbox it was sent from, where the reconcile + * pipeline (`src/mail/gmail-reconcile.ts`) ingests it like any other inbound + * message. `src/mail/ingest.ts`'s loop guard (`isOwnMessageReflection`) only + * recognizes a reflection whose OWN `Message-ID` is our token — but Gmail + * rewrites the wire `Message-ID` (this file's own module doc, above), so the + * guard never fires for this transport. Without a second guard, that + * self-echo carries our valid token as the LAST `References` entry, + * `decideThreading` finds it and returns `'append'`, and the agent's own + * reply gets stored a second time as a phantom `direction: 'inbound'` + * message in the very conversation it belongs to — reopening it if it was + * closed (`appendThreadInTx`, `src/store/conversations.ts`). + * + * `selfEchoGuard` (optional — {@link SelfEchoGuardDeps}) closes this WITHOUT + * touching `decideThreading` or adding a threading heuristic: immediately + * after a successful send, if the sender returned a `providerMessageId` + * (`EmailSendResult.providerMessageId` — Gmail's `body.id`, the SAME id + * `gmail-reconcile.ts` will later see for this exact message during + * `history.list`), this module resolves `input.from` to its `MailboxRecord` + * (`MailboxStore.getMailboxByAddress`) and pre-seeds `(mailboxId, + * providerMessageId)` as an already-`suppressed` row in the inbound delivery + * ledger (`InboundDeliveryStore.preSuppressOwnSend`, `src/store/inbound- + * deliveries.ts`). When reconcile later lists that SAME provider id and + * calls `ingestInboundMessage`, its `claim()` finds the pre-seeded + * `suppressed` row and reports the terminal outcome as-is — the existing + * "do not double-process a terminal row" path, never a new code path in + * `ingest.ts`. This is best-effort and never affects the send's own outcome + * (the message is already delivered by the time this runs) — see {@link + * suppressSelfEcho}'s doc comment for the failure modes this accepts. */ import { randomUUID } from 'node:crypto' import type { EmailSender } from '../providers/index.js' import type { ConversationStore, SendEnvelope, StoredThread } from '../store/conversations.js' +import type { InboundDeliveryStore } from '../store/inbound-deliveries.js' +import type { MailboxStore } from '../store/mailboxes.js' import { injectTrackingPixel, mintViewToken, pixelUrlFor } from './open-tracking.js' import { type Keyring, mintReplyMessageId } from './reply-token.js' @@ -170,6 +241,21 @@ export function assertLeaseExceedsSenderBound(sender: EmailSender, leaseMs: numb } } +/** + * Dependencies for the self-echo guard (module doc's "The reply token's own + * self-echo" section, HT-49 review fix). Optional in {@link SendReplyDeps} — + * a deployment with no Gmail (or other self-reflecting) transport configured + * simply never sets this, and every existing test/caller is unaffected: with + * it absent, {@link suppressSelfEcho} is a complete no-op, byte-identical to + * before this guard existed. + */ +export interface SelfEchoGuardDeps { + /** Resolves `SendReplyInput.from` to the mailbox it belongs to (`MailboxStore.getMailboxByAddress`). */ + mailboxStore: MailboxStore + /** Where the pre-seeded suppression row is written (`InboundDeliveryStore.preSuppressOwnSend`). */ + inboundDeliveryStore: InboundDeliveryStore +} + /** Dependencies `sendReply` needs, injected so it stays testable against fakes/in-memory stores. */ export interface SendReplyDeps { store: ConversationStore @@ -190,6 +276,8 @@ export interface SendReplyDeps { * pixel with no extra logic. */ openTracking?: { publicBaseUrl: string } + /** See {@link SelfEchoGuardDeps}. ABSENT BY DEFAULT — see that interface's doc comment. */ + selfEchoGuard?: SelfEchoGuardDeps } /** One outbound reply to an existing conversation (specs/mail/sending.md §5: reply-only in this increment). */ @@ -203,7 +291,15 @@ export interface SendReplyInput { html?: string /** `In-Reply-To` of the inbound message being answered — caller-supplied (specs/mail/sending.md §5). */ inReplyTo?: string - /** `References` chain of the inbound message being answered — caller-supplied (specs/mail/sending.md §5). */ + /** + * `References` chain of the inbound message being answered — caller-supplied + * ANCESTOR ids only (specs/mail/sending.md §5). `sendReply` appends this + * call's own freshly-minted `messageId` as the FINAL entry before sending or + * persisting (HT-49; see the module doc's "References carries the reply + * token" section) — this field should never itself include the reply's own + * id, and the outbound `References` actually transmitted is always this + * array plus one more entry, even when this field is omitted entirely. + */ references?: string[] /** * Optional caller-supplied dedup key (HT-16), scoped per-conversation. See @@ -279,14 +375,23 @@ export async function sendReply( keyring, ) - // Open tracking (spec §4g): with the feature OFF (the default), `input` + // HT-49: append this reply's OWN minted messageId as the FINAL References + // entry, after any ancestor ids the caller supplied — see the module doc's + // "References carries the reply token" section for why. Unconditional and + // always non-empty (even a first reply with no ancestors gets a one-element + // References: [messageId]): the token needs this durable channel onto the + // wire regardless of how many ancestors precede it. + const references = [...(input.references ?? []), messageId] + + // Open tracking (spec §4g): with the feature OFF (the default), the body // passes through UNTOUCHED — this line is the whole off-path, and the // byte-identical-mail guarantee rests on it. With it on, only the HTML // body changes, before persist (see SendReplyDeps.openTracking). On a // keyed REPLAY the modified body is irrelevant either way — appendThread - // returns the ORIGINAL row's persisted body (§4a's replay rule). - const effectiveInput: SendReplyInput = - deps.openTracking !== undefined && input.html !== undefined + // returns the ORIGINAL row's persisted body (§4a's replay rule). References + // is always overridden to the HT-49 chain above, independent of tracking. + const effectiveInput: SendReplyInput = { + ...(deps.openTracking !== undefined && input.html !== undefined ? { ...input, html: injectTrackingPixel( @@ -294,18 +399,21 @@ export async function sendReply( pixelUrlFor(deps.openTracking.publicBaseUrl, mintViewToken(threadId, keyring)), ), } - : input + : input), + references, + } // The envelope snapshot is built from THIS call's inputs and persisted // verbatim on insert, keyed or not — persisting it unconditionally (not // only when idempotencyKey is set) is what lets the delivery worker // reconstruct ANY eligible outbound row later, regardless of whether its - // original send carried a dedup key. + // original send carried a dedup key. `references` (never `input.references`) + // is always set, per HT-49 above. const sendEnvelope: SendEnvelope = { to: input.to, ...(input.cc !== undefined ? { cc: input.cc } : {}), subject: input.subject, - ...(input.references !== undefined ? { references: input.references } : {}), + references, } const appended = await store.appendThread(input.conversationId, { @@ -390,7 +498,11 @@ export async function sendReply( return { ok: false, reason: 'retry-in-progress' } } - return attemptDeliveryOfClaimedThread(claimed, { store, sender }) + return attemptDeliveryOfClaimedThread(claimed, { + store, + sender, + selfEchoGuard: deps.selfEchoGuard, + }) } /** @@ -409,9 +521,10 @@ async function sendFreshAndMark( deps: SendReplyDeps, ): Promise { const { store, sender } = deps + let sendResult: Awaited> try { - await sender.send({ + sendResult = await sender.send({ messageId, inReplyTo: input.inReplyTo, references: input.references, @@ -457,9 +570,51 @@ async function sendFreshAndMark( markErr, ) } + await suppressSelfEcho(input.from, sendResult.providerMessageId, deps.selfEchoGuard) return { ok: true, threadId, messageId, delivery: 'sent' } } +/** + * Best-effort: pre-seed the pending self-echo of this JUST-SENT message as + * suppressed in the inbound delivery ledger — module doc's "The reply + * token's own self-echo" section. A no-op when `guard` is absent (no + * self-reflecting transport configured) or `providerMessageId` is absent + * (the sender didn't report one — nothing to correlate against later). + * + * Deliberately never throws: this runs AFTER the message is already + * delivered (`sendFreshAndMark`/`attemptDeliveryOfClaimedThread` only call it + * once `sender.send()` has resolved), so a failure here must never turn an + * already-successful send into a reported failure — it would invite a + * resend of mail that already went out. Losing this race (or a lookup/write + * error) just means the pre-HT-49-fix failure mode (a phantom inbound + * self-echo, if this transport reflects sent mail back to its own mailbox) + * can still occur for this one send — logged, not silently swallowed. + */ +async function suppressSelfEcho( + fromAddress: string, + providerMessageId: string | undefined, + guard: SelfEchoGuardDeps | undefined, +): Promise { + if (guard === undefined || providerMessageId === undefined) return + + try { + const mailbox = await guard.mailboxStore.getMailboxByAddress(fromAddress) + if (mailbox === null) return + await guard.inboundDeliveryStore.preSuppressOwnSend( + mailbox.id, + providerMessageId, + 'own-outbound-self-echo', + ) + } catch (err) { + console.error( + "[sendReply] failed to pre-suppress this send's self-echo in the inbound delivery " + + 'ledger; if this transport reflects sent mail back into its own mailbox, reconcile may ' + + 'ingest it as a phantom inbound message (HT-49)', + err, + ) + } +} + /** * Attempt delivery of an ALREADY-CLAIMED outbound row, then mark * `sent`/`failed` and release its lease. Shared by {@link sendReply}'s @@ -482,7 +637,7 @@ async function sendFreshAndMark( */ export async function attemptDeliveryOfClaimedThread( thread: StoredThread, - deps: { store: ConversationStore; sender: EmailSender }, + deps: { store: ConversationStore; sender: EmailSender; selfEchoGuard?: SelfEchoGuardDeps }, ): Promise< | { ok: true; threadId: string; messageId: string; delivery: 'sent' } | { @@ -502,9 +657,10 @@ export async function attemptDeliveryOfClaimedThread( } const messageId = thread.messageId const envelope = thread.sendEnvelope + let sendResult: Awaited> try { - await sender.send({ + sendResult = await sender.send({ messageId, inReplyTo: thread.inReplyTo ?? undefined, references: envelope.references, @@ -548,5 +704,6 @@ export async function attemptDeliveryOfClaimedThread( markErr, ) } + await suppressSelfEcho(thread.fromAddress, sendResult.providerMessageId, deps.selfEchoGuard) return { ok: true, threadId: thread.id, messageId, delivery: 'sent' } } diff --git a/src/providers/adapters/gmail/mime.test.ts b/src/providers/adapters/gmail/mime.test.ts index ae18dc7..aaa0eb7 100644 --- a/src/providers/adapters/gmail/mime.test.ts +++ b/src/providers/adapters/gmail/mime.test.ts @@ -67,6 +67,28 @@ describe('buildRawMessage', () => { expect(unfold(raw)).toContain(`References: ${references.join(' ')}`) }) + // HT-49: send.ts appends the reply's OWN minted messageId as the FINAL + // References entry (after ancestor ids) — the durable channel for the + // reply token once a provider (Gmail, confirmed live) rewrites Message-ID + // on send. This wire-level test locks that ordering is preserved verbatim + // by buildRawMessage, and that In-Reply-To (which still names the + // ANCESTOR being answered, never this reply's own id) is untouched. + it('HT-49: the reply token, appended as the final References entry, survives as the LAST entry on the wire; In-Reply-To is unchanged', () => { + const inReplyTo = '' + const references = ['', messageId] + + const raw = buildRawMessage({ ...base, text: 'body', inReplyTo, references }) + + expect(raw).toContain(`In-Reply-To: ${inReplyTo}`) + expect(raw.match(/In-Reply-To:/g)).toHaveLength(1) + // Unfolded References ends with our token, exactly the given order. + const referencesLine = unfold(raw) + .split('\r\n') + .find((l) => l.startsWith('References:')) + expect(referencesLine).toBe(`References: ${references.join(' ')}`) + expect(referencesLine?.endsWith(messageId)).toBe(true) + }) + it('omits In-Reply-To and References entirely when not supplied', () => { const raw = buildRawMessage({ ...base, text: 'body' }) diff --git a/src/providers/email-sender.ts b/src/providers/email-sender.ts index 492a81f..2ca36ac 100644 --- a/src/providers/email-sender.ts +++ b/src/providers/email-sender.ts @@ -9,26 +9,47 @@ * ## The `Message-ID` contract is load-bearing * * Outbound-anchored threading (specs/mail/threading.md §2, specs/mail/sending.md - * §1) only works if the `Message-ID` a customer's reply eventually echoes - * back in `In-Reply-To`/`References` is EXACTLY the signed-token id the - * engine minted (`mintReplyMessageId`, `src/mail/reply-token.ts`) — not a + * §1) is designed around the `Message-ID` a customer's reply eventually + * echoes back in `In-Reply-To`/`References` being EXACTLY the signed-token id + * the engine minted (`mintReplyMessageId`, `src/mail/reply-token.ts`) — not a * provider-generated substitute. So every `EmailSender` implementation MUST * transmit `OutboundEmail.messageId` **verbatim** as the RFC 5322 * `Message-ID` header, and MUST NOT generate or overwrite it with a * provider-assigned id. `inReplyTo` and `references`, when present, are - * likewise engine-set (specs/mail/sending.md §5: caller-supplied from the - * inbound message being answered) and must be transmitted as given, not - * reinterpreted. A provider SDK that cannot set `Message-ID` explicitly + * likewise engine-set (specs/mail/sending.md §5: caller-supplied ancestor ids + * plus the reply's own minted id, HT-49) and must be transmitted as given, + * not reinterpreted. A provider SDK that cannot set `Message-ID` explicitly * (some transactional-email APIs only expose a "reply-to" concept and mint * their own `Message-ID` unconditionally) is unusable for Helpthread and * must not be adapted to this interface — there is no fallback path that * preserves threading correctness. * + * **This is necessary, but — live evidence, HT-49 — not always sufficient.** + * An adapter that faithfully transmits `Message-ID` verbatim (satisfying the + * contract above) can still have it silently REWRITTEN by the provider's own + * infrastructure after transmission: Gmail's `users.messages.send` has been + * observed doing exactly this in production (2026-07-17), substituting its + * own generated id on the wire despite the Gmail adapter setting the engine's + * token correctly. This is not a contract violation — it happens downstream + * of what any adapter controls — but it is why `References` also carries the + * token as a provider-durable backup channel (specs/mail/threading.md §2a); + * see there for the full mechanism. + * * `EmailSendResult.providerMessageId` is a SEPARATE, optional field for the * provider's own internal delivery id (e.g. for looking up delivery status * or bounce webhooks in that provider's dashboard/API later). It carries no * threading authority and is never compared against `messageId` — the two * ids serve entirely different purposes and must not be confused. + * + * As of HT-49's review fix, `src/mail/send.ts` ALSO reads this field for one + * additional purpose that is likewise not threading authority: when present, + * it pre-seeds this exact send's self-echo as suppressed in the inbound + * delivery ledger (`InboundDeliveryStore.preSuppressOwnSend`), because it is + * the SAME id a self-reflecting transport (Gmail, confirmed live) later + * reports for that message during reconcile — see `src/mail/send.ts`'s "The + * reply token's own self-echo" section and `specs/mail/inbound-ingestion.md` + * §5's HT-49 amendment. This is a ledger dedup key, not a threading + * correlation — `decideThreading` still never reads it. */ /** One fully-formed outbound email, ready to transmit. */ diff --git a/src/store/inbound-deliveries.test.ts b/src/store/inbound-deliveries.test.ts index fea8ed0..d0890c3 100644 --- a/src/store/inbound-deliveries.test.ts +++ b/src/store/inbound-deliveries.test.ts @@ -391,4 +391,48 @@ describe('createInboundDeliveryStore', () => { db.transaction(async (tx) => markStoredInTx(tx, RANDOM_UUID, RANDOM_UUID, 0)), ).rejects.toThrow(/no delivery with id/) }) + + // --- HT-49 review fix: preSuppressOwnSend -------------------------------- + + it('preSuppressOwnSend on a fresh key creates an already-suppressed row that claim() then reports as terminal, never re-ingesting it', async () => { + const { store, mailboxId } = await freshStore() + + await store.preSuppressOwnSend(mailboxId, 'gmail-self-echo-1', 'own-outbound-self-echo') + + const result = await store.claim(mailboxId, 'gmail-self-echo-1', LEASE_MS) + + expect(result.claimed).toBe(false) + expect(result.delivery).toMatchObject({ + mailboxId, + providerMessageId: 'gmail-self-echo-1', + status: 'suppressed', + lastError: 'own-outbound-self-echo', + }) + }) + + it('preSuppressOwnSend never overwrites a row a genuine claim() already won (the race is conceded, not corrected)', async () => { + const { store, mailboxId } = await freshStore() + + // A genuine concurrent ingest claims this key FIRST... + const claimed = await store.claim(mailboxId, 'provider-msg-1', LEASE_MS) + expect(claimed.claimed).toBe(true) + + // ...then the self-echo guard loses the race and tries to pre-seed the + // SAME key as suppressed. + await store.preSuppressOwnSend(mailboxId, 'provider-msg-1', 'own-outbound-self-echo') + + // The already-`received` row is untouched — never silently flipped. + const replay = await store.claim(mailboxId, 'provider-msg-1', LEASE_MS) + expect(replay).toMatchObject({ claimed: false, delivery: { status: 'received' } }) + }) + + it('preSuppressOwnSend is a silent no-op when the key is already suppressed', async () => { + const { store, mailboxId } = await freshStore() + + await store.preSuppressOwnSend(mailboxId, 'provider-msg-1', 'own-outbound-self-echo') + await store.preSuppressOwnSend(mailboxId, 'provider-msg-1', 'own-outbound-self-echo') + + const result = await store.claim(mailboxId, 'provider-msg-1', LEASE_MS) + expect(result).toMatchObject({ claimed: false, delivery: { status: 'suppressed' } }) + }) }) diff --git a/src/store/inbound-deliveries.ts b/src/store/inbound-deliveries.ts index 522f6a8..f7ca7a3 100644 --- a/src/store/inbound-deliveries.ts +++ b/src/store/inbound-deliveries.ts @@ -147,6 +147,27 @@ * spec §5 asks for — "recorded in the ledger (suppressed, with the reason)" * — just sharing a column with the failure-path's error text rather than * owning a dedicated one. + * + * ## Pre-seeded suppression (HT-49 review fix): suppressing before a claim exists + * + * Every mark* method above requires a row already `claim()`-ed to `received` + * — the ordinary "ingest ran, then decided to suppress" order. {@link + * InboundDeliveryStore.preSuppressOwnSend} is the one exception: it creates + * an ALREADY-`suppressed` row from scratch, before any `claim()` for that key + * has ever happened. This exists for exactly one caller, `src/mail/send.ts`'s + * self-echo guard (see that module's doc comment): some transports (Gmail + * confirmed — HT-49 live evidence) deliver the sent copy of an outbound + * reply back into the SAME mailbox it was sent from, where the reconcile + * pipeline (`src/mail/gmail-reconcile.ts`) would otherwise ingest it as a + * genuine new inbound message — and by the time that happens, the token this + * fix added to `References` (threading.md §2a) makes that self-echo `append` + * to the very conversation it belongs to, duplicating the agent's own reply + * as a phantom customer message. Pre-seeding `(mailboxId, + * providerMessageId)` — using the SAME provider id (`EmailSendResult. + * providerMessageId`) the transport will later report for that exact message + * during reconcile — means `claim()`'s ordinary "terminal row, do not + * double-process" branch absorbs the echo with zero heuristics and zero + * changes to `decideThreading`. */ import type { Db, Queryable } from '../db/client.js' @@ -234,6 +255,28 @@ export interface InboundDeliveryStore { * `Error` if no row exists with `id` at all. */ markDeadLetter(id: string, error: string, claimedAttempts: number): Promise + + /** + * Pre-seed `(mailboxId, providerMessageId)` as ALREADY `suppressed`, + * before any `claim()` for that key has happened — see the module doc's + * "Pre-seeded suppression" section for why this exists and who calls it. + * + * A plain `INSERT ... ON CONFLICT (mailbox_id, provider_message_id) DO + * NOTHING` — there is no row to `RETURNING`, and nothing for the caller to + * act on either way. If a row ALREADY exists for this key — the race where + * a reconcile run's `claim()` won first, ingesting the message before this + * call could pre-seed the suppression (module doc) — this is a SILENT + * no-op: whatever status that row already reached (`received`, `stored`, + * or `suppressed` from a genuine concurrent path) is left completely + * untouched. This method must NEVER overwrite an existing row: doing so + * could silently flip an already-committed `stored` row (with its own + * `thread_id` a conversation now depends on) to `suppressed`, corrupting a + * message that merely happened to reuse this `providerMessageId` first. + * Losing this race reproduces the pre-HT-49-fix failure (a phantom inbound + * self-echo) rather than a NEW one — a known, accepted residual (see the + * caller's doc comment), not silently hidden. + */ + preSuppressOwnSend(mailboxId: string, providerMessageId: string, reason: string): Promise } /** @@ -412,6 +455,18 @@ export function createInboundDeliveryStore(db: Db): InboundDeliveryStore { ) return oneOrFenced(db, rows, 'markDeadLetter', id) }, + + async preSuppressOwnSend(mailboxId, providerMessageId, reason) { + // No RETURNING, no fence — see the interface doc comment. A conflict + // means another path (an ordinary claim()) already owns this key; + // this call must never touch that row. + await db.query( + `INSERT INTO inbound_deliveries (mailbox_id, provider_message_id, status, last_error) + VALUES ($1, $2, 'suppressed', $3) + ON CONFLICT (mailbox_id, provider_message_id) DO NOTHING`, + [mailboxId, providerMessageId, reason], + ) + }, } }