Repository navigation
feat(email-core): guard against caller-replayed duplicate delivery - #209
Merged
Merged
Conversation
`isRetryable(err, 'delivery') === false` and the single-attempt dispatch in
send_email/reply_to_email/send_draft guarantee that this library never
re-issues a send. They guarantee nothing about the caller, and the caller is
the one that replays.
The residual hole: an agent whose tool result was lost — a compacted context, a
dropped transport, a supervisor restarting the turn, a person retrying a call
that looked stuck — re-issues the identical instruction, and the second call is
indistinguishable from a first one. The only defence was a sentence of English
in each tool description ("do not resend without checking Sent Items"), which
is exactly the guard an LLM under retry pressure skips. When it fails it fails
silently: the second send succeeds and the recipient gets the message twice.
Provider endpoints take no idempotency key, so this cannot be fixed at the
provider boundary. It is fixed one level up, with the state nothing previously
held: an in-process ledger keyed by a digest of what would actually be
delivered — mailbox, recipients, subject, rendered post-truncation body,
attachment content hashes, scheduled time, and the reply parent or draft id.
The record is written BEFORE dispatch, so the reserved window is exactly the
window in which an acknowledgement gets lost, and settled after.
Three refusal codes rather than one, because the recovery differs:
DUPLICATE_SEND_IN_FLIGHT (wait), DUPLICATE_SEND_BLOCKED (stop — the prior
messageId comes back with it), DUPLICATE_SEND_UNRESOLVED (a human checks Sent
Items).
Release is an allowlist, not a denylist. Only codes that PROVE non-delivery —
4xx-derived, 429, connect-failure — free the fingerprint, so a resend after a
rejection is never blocked. Everything else, including a code no provider has
shown us yet, is held. A wrongly-held message costs one refused resend and
prints the override; a wrongly-released one costs a duplicate in somebody's
inbox, which is the bug being fixed.
On by default, with no embedder wiring. ActionContext.sendLedger exists only so
tests can inject an isolated ledger; when absent the actions fall back to the
process default. ActionContext.rateLimiter is the cautionary example — an
optional injected interface that no shipped adapter constructs, so the rate
limit it documents does not exist in the product. A guard someone has to
remember to wire is a guard that is off.
send_draft claims before the draft lookup, not after: a provider that consumed
the draft on the first send makes the second lookup fail, and "cannot verify
draft recipients" is a misleading answer to "did this already go out?". Every
bail-out between claim and dispatch releases.
allow_duplicate: true is the deliberate escape hatch, and re-arms the guard
against its own replay.
Limitation, stated rather than papered over: the ledger is per-process and in
memory. It closes the replay window that actually occurs — a retry inside a
running server — and does not survive restarting that server. Cross-process
durability needs a persisted store and is deliberately not attempted here.
Evidence: 559 email-core tests pass (up from 546). Negative control run —
rebuilding the suite with the ledger window forced to 0 turns 6 of the new
assertions red and leaves the three that assert non-blocking behavior green, so
the tests can fail. A separate check drives the built dist through an
ActionContext with no sendLedger and confirms the unwired production path is
guarded; it is kept as the "Guard is active without embedder wiring" test
rather than as a one-off script.
Ref: openspec/changes/add-duplicate-send-guard
Claude-Session: https://claude.ai/code/session_01HkdNf9x4QS4LYS2DSiQWXS
Codex peer review found five executable defects in the duplicate-send guard. Each was reproduced against the tree before being fixed, and each now has a regression test that fails without the fix. 1. Settle and release addressed only the fingerprint, so they acted on whatever attempt occupied the key. A fingerprint can legitimately carry two live attempts, because allow_duplicate creates one. Interleaving: A dispatches; a forced B dispatches and succeeds; A comes back rejected and DELETES B's successful record; an ordinary replay then sends a third copy. Attempts now carry an identity and the claim closure holds its own entry. 2. A forced claim discarded the prior record before the forced send was known to have delivered, so a rejected override erased the delivery it was overriding and the next ordinary call sent again. No concurrency needed. The forced attempt is now recorded alongside the earlier one. Same defect at the action layer in send_draft: forcing a consumed draft failed its lookup, released, and left an ordinary replay reporting DRAFT_LOOKUP_FAILED instead of naming the message that had already gone out. 3. The process-default ledger was keyed on ctx.mailboxName, which is optional. Two mailboxes in one process shared the empty key: the second mailbox's FIRST EVER send was refused and handed the first mailbox's message id — a wrong refusal and a cross-mailbox identifier leak in one response. The namespace now falls back to a per-provider-instance id held in a WeakMap. The shipped MCP wrapper always supplies a resolved name, so this was the library and default-context surface, not that server. 4. A success that settled after its reservation had been pruned was dropped on the floor, admitting an immediate replay of a message that had just been delivered. A late settlement is re-inserted and dated from settlement. 5. Size-cap eviction could evict an in-flight reservation — dropping cover for a send that is on the wire, the worst thing this module can do. Eviction now takes the oldest SETTLED attempt and never an active one. A burst of concurrent sends may briefly carry the map past the cap; refusing a claim because the ledger is full would block first-ever sends, which is worse. Also from the review, verified at each production site: - DELIVERY_PROVEN_UNSENT_CODES was incomplete for the shipped providers. Graph REPLY_FAILED (prepareReplyDraft throws before /send), ATTACHMENT_TOO_LARGE_FOR_PROVIDER (size validation, zero POSTs), ATTACHMENT_UPLOAD_FAILED (draft created, /send never called) and SCHEDULE_DRAFT_FAILED (no draft id, no deferred-send time) all provably deliver nothing, yet each held a fingerprint and produced a false refusal. - Gmail reported a delivery whose connection never opened as generic PROVIDER_ERROR, which cannot be released because the same code also covers failures that are not proven. It now reports PROVIDER_UNREACHABLE for that transport condition, matching what the Graph provider already does. Generic PROVIDER_ERROR is deliberately NOT added to the allowlist. Documentation corrections, including one claim that was false when written: - The proposal said the ledger stores a "salted digest". No salt was ever implemented. Corrected in place with a note saying what it previously claimed, rather than quietly deleted. - The spec said a colliding claim SHALL NOT "invoke the provider". Too broad: reply_to_email reads the parent message before consulting the ledger, and send_draft reads the draft. It is the delivery dispatch that is suppressed. - The override was described as replacing the prior record. It is recorded alongside it, and allow_duplicate is an unconditional per-request bypass — a replay that itself carries the flag dispatches again. Nothing here makes the forced request idempotent. - The README described the fingerprint as the bytes that leave the process. Providers transform outbound mail (Graph truncates a subject at 255), so it is the effective action inputs. The guard catches a replayed tool call. Evidence: 569 email-core tests and 119 provider-gmail tests pass, up from 559 and 116. Codex's own 13 executable probes go from 1 passing to 11; the two that remain are the ones it classified as out of scope for this commit — the pre-existing scheduled-send fallback code, fixed separately next, and the provider-truncation equivalence, which is addressed as wording above. Ref: openspec/changes/add-duplicate-send-guard Ref: .peer-review/dup-send-guard-1788823130-24309-codex.md Claude-Session: https://claude.ai/code/session_01HkdNf9x4QS4LYS2DSiQWXS
Scheduling is a two-write draft→send. When the provider itself classifies a failure the action passes that code through, but the catch-all fallback for an unclassified throw was SCHEDULE_SEND_FAILED — a terminal code asserting the submission was rejected, for an outcome that proves nothing of the kind. handleProviderError already states the rule in its own doc comment: for a delivery operation the fallback MUST be an ambiguous code, because an unclassified failure on a send is exactly the case where we cannot prove the message did not go out. The immediate-send path follows it; the scheduled path did not. A caller told "failed" for a submission that may have been queued is being invited to schedule a duplicate. The fallback becomes SCHEDULE_SEND_STATUS_UNKNOWN, and quota is charged on it for the same reason the non-throw branch two lines above already charges it. SCHEDULE_SEND_FAILED is unchanged where the Microsoft provider classifies a 4xx rejection — that code is still correct and still proves non-delivery. Found during Codex peer review of the duplicate-send guard and confirmed to predate it, so it lands as its own commit rather than inside that change. The guard was already correct here regardless: an unclassified throw settles with no code and is held as unresolved, so a replay was refused either way. This fixes what the CALLER is told. Tests: the ambiguous fallback, plus a negative control asserting a provider-classified SCHEDULE_SEND_FAILED still surfaces as terminal — without it the fix could have blurred the distinction it exists to preserve. Claude-Session: https://claude.ai/code/session_01HkdNf9x4QS4LYS2DSiQWXS
stevenobiajulu
enabled auto-merge (squash)
September 8, 2026 00:30
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Member
Author
✅ Post-merge smoke passedMerged: Steps
Feature end-to-end Two halves, both against the artifact this merge produced rather than the source tree. Shipped MCP surface — spawned
Guard behaviour — the built
Synthetic identities only ( Visual evidence: n/a — no user-visible rendered output. Advisory-gate audit: no bot/gate comments and zero legacy commit-statuses on the head SHA, so nothing green-but-degraded is hiding behind a passing check. All four required contexts ( Cleanup
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
isRetryable(err, 'delivery') === falseand the single-attempt dispatch insend_email/reply_to_email/send_draftguarantee that this library never re-issues a send. They guarantee nothing about the caller, and the caller is the one that replays.An agent whose tool result was lost — a compacted context, a dropped transport, a supervisor restarting the turn, a person retrying a call that looked stuck — re-issues the identical instruction, and the second call is indistinguishable from a first one. The only defence was a sentence of English in each tool description ("do not resend without checking Sent Items"), which is exactly the guard an LLM under retry pressure skips. When it fails it fails silently: the second send succeeds and the recipient gets the message twice.
Provider endpoints take no idempotency key, so this cannot be fixed at the provider boundary. It is fixed one level up.
What
An in-process send ledger keyed by a digest of the effective action inputs — mailbox, recipients, subject, rendered post-truncation body, attachment content hashes, scheduled time, reply parent or draft id. The record is written before dispatch, so the reserved window is exactly the window in which an acknowledgement gets lost, and settled after.
Three refusal codes, because the recovery differs:
DUPLICATE_SEND_IN_FLIGHTDUPLICATE_SEND_BLOCKEDmessageIdDUPLICATE_SEND_UNRESOLVEDRelease is an allowlist, not a denylist. Only codes that prove non-delivery — 4xx-derived, 429, connect-failure, and the Graph paths that fail before the delivery POST — free the fingerprint, so a resend after a rejection is never blocked. Everything else, including a code no provider has shown us yet, is held. A wrongly-held message costs one refused resend and names the override; a wrongly-released one costs a duplicate in somebody's inbox.
On by default, with no embedder wiring.
ActionContext.sendLedgerexists only so tests can inject an isolated ledger.ActionContext.rateLimiteris the cautionary example — an optional injected interface that no shipped adapter constructs, so the rate limit it documents does not exist in the product.allow_duplicate: trueis the deliberate escape hatch;AGENT_EMAIL_DUPLICATE_SEND_WINDOW_MStunes the window (default 15 min,0disables).Commits
feat(email-core)— the guard.fix(email-core)— five defects Codex peer review found in it, each reproduced before being fixed and each now carrying a regression test that fails without the fix: settlement/release addressed the fingerprint rather than the attempt (a slow attempt's late rejection deleted a newer attempt's successful record); a rejectedallow_duplicateerased the delivery it was overriding; the process-default ledger keyed on an optionalmailboxName, so a second mailbox's first-ever send was refused and handed the first mailbox's message id; a success settling after its reservation was pruned was dropped; size-cap eviction could evict an in-flight reservation. Plus incomplete proven-unsent codes for both shipped providers, and four documentation claims that overstated what the code does — including a "salted digest" that was never implemented, corrected in place rather than quietly deleted.fix(email-core)— a pre-existing misreport, found during that review and confirmed to predate the guard: an unclassified throw on the scheduled-send path was labelledSCHEDULE_SEND_FAILED, a terminal code, for an outcome that proves nothing of the kind.Limitation, stated rather than papered over
The ledger is per-process and in memory. It closes the replay window that actually occurs — a retry inside a running server — and does not survive restarting that server. Cross-process durability needs a persisted store and is deliberately not attempted here.
Evidence
All four gates green:
build,lint,test:run,check:spec-coverage(290/290). 571 email-core tests (from 546) and 119 provider-gmail tests (from 116).Two mutation controls, because a green check that could not have gone red proves nothing:
0(guard off) → 7 of the new assertions go red; the ones asserting non-blocking behavior correctly stay green.Codex's own 13 executable probes go from 1 passing to 12. The one that remains is the provider-truncation equivalence it classified itself as a scope/wording issue — Graph truncates a subject at 255 chars, so two different inputs can leave as identical mail; addressed by correcting the README to describe the fingerprint as effective action inputs rather than final wire bytes.
OpenSpec change:
openspec/changes/add-duplicate-send-guard(validates--strict, 13 scenarios each backed by a matching test name).https://claude.ai/code/session_01HkdNf9x4QS4LYS2DSiQWXS