feat(compose): add verifiable delivery receipts - #403
Closed
jonata97 wants to merge 5 commits into
Closed
Conversation
`hey compose --json` answered `data: null`. The SDK's `Messages().Create` and
`Entries().CreateReply` both do their POST and return an error and nothing else, so the
response — its status, its `Location`, its body — went on the floor, and with it the only
thing naming the message that had just been created. "The server said 2xx" is not
evidence that a particular message exists, and this endpoint carries no idempotency key,
so a retry on that basis can deliver twice.
compose_send.go posts a send through `Client.PostMutation` with the SDK's own generated
request types, so the wire contract stays defined once in the SDK and this is a
response-preserving wrapper rather than a second definition of the API. The durable home
is a `CreateWithResult` on the SDK's services; when there is one, these two functions go.
handleFromResponse mines that response for a handle and is strict on purpose: every
accepted shape has a fixture, and everything else is an error. It reads the `Location`
header — which is how a saved draft already answers, `204` with `…/messages/{entry_id}`,
from the same controllers — and a JSON body naming `id`, `message_id`, `message.id`,
`topic_id` or `app_url`. A response naming neither is `apierr.ErrAmbiguousOutcome`: not
`api`, not `network`, because both of those read as "it did not happen" and this one does
not. The failure mode of a lenient parser is reporting a message as sent when nothing can
show which one it was, which is worse than saying we do not know, because a caller told
"sent" stops looking.
verifyComposedMessage then reads the message back with `Messages().Get` — the same request
`hey thread read` makes per entry, so the two report the same conversion of the same
record — and answers `verified`, `mismatch` or `unverified`. `mismatch` is never a reason
to send again: the message exists. The body is compared as canonical Markdown rather than
as HTML, since HEY may reformat the markup it stores but not what the message says, and a
body past the inline bound is left out entirely rather than published in part, with its
digest still covering the whole of it.
`matches_sent.recipients` is one-sided on BCC, because HEY serves no blindcopied line back
for a delivered message; `bcc_disclosed` says so rather than an empty list being passed off
as proof. What it still refuses is a recipient nobody asked for.
`hey thread read --json` carries the same envelope per entry — `subject`, `sender` and
`addressed` — off the same message record, so a caller can identify the exact entry
without inferring recipients by position or by searching the body. threadload's `retained`
keeps `Sender` and `Addressed` on the message it holds, charged to the byte budget like
everything else it keeps, which is what bounds a thread addressed to a mailing list; it
used to drop them. Per-message publication is bounded separately at
`maxRetainedRecipients`, and a cut list says it was cut.
The reply and attachment fixtures now answer a send the way HEY does — with a Location
naming the entry — rather than with a body that carries nothing.
The smoke test's cross-verification used to sit inside `if appURL, ok := …`, so a compose
returning no handle passed silently. The handle and the verification block are now
required, and `hey thread read` is asked for the same envelope back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…body was on it `bcc_disclosed` was `len(bcc) > 0`, so one shape — `bcc: [], bcc_disclosed: false` — stood for two different facts: HEY served an empty blindcopied line, which is proof that nobody was blind-copied, and HEY withheld the line, which proves nothing. A caller that has to establish a message's exact destination set could not tell those apart, so it could not report success on either. It is presence now, not population: true when the field arrived — an explicitly empty array included — and false when it was omitted, null, or there was no addressing at all. The distinction survives the decode without help: encoding/json leaves an omitted or null array nil and makes `[]` non-nil, generated.Message declares no unmarshaler of its own, and threadload's retained copies the slice header rather than rebuilding it. So the fix is one line reading `addressed.Blindcopied != nil` instead of the length of the filtered output. That invariant is the kind that breaks in silence, so it is pinned through the real path — an HTTP response decoded by the SDK — rather than by building a struct by hand: TestBlindcopiedPresenceSurvivesTheSDKDecode covers omitted, null, `[]` and populated. The same matrix runs end to end through `hey thread read --json` and `hey compose --json`, so both reads are held to one meaning. Disclosure and the bound stay independent: a line long enough to be cut was plainly served, so it is disclosed and truncated at once, and a served line whose only contact carries no address is still disclosed — HEY answered the question, the answer just has nothing to print. `matches_sent.recipients` is unchanged and still asks only whether an unexpected recipient came back. Whether a requested BCC must also appear is the caller's policy, and it now has the two facts it needs to apply one: the exact BCC values and whether they are evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… as a network error
A bounded probe had a server read one complete POST and then drop the connection
without answering. compose reported code `network`, no hint, exit 6 — a taxonomy that
reads as "it did not happen" and invites a retry. HEY had the request. Whether it
delivered the message is unknowable from here, and this endpoint carries no idempotency
key, so retrying on that signal can put the same mail in somebody's inbox twice.
classifySendFailure now stands at the two send call sites and asks one question: does
this failure prove HEY did not act? Two things prove it — the request was never
dispatched, or HEY answered with a status that is itself a refusal. Everything else is
apierr.ErrAmbiguousOutcome (exit 8) with a hint saying to read the thread back rather
than send again.
The ambiguous case is the default rather than a special case, because the two failure
modes are not symmetric: reading `network` off a completed send and retrying delivers
twice, while reading `ambiguous` off a send that never happened costs a look at an empty
thread. Only one of those is recoverable, so an outcome the classifier cannot recognise
is one it will not rule out.
never dispatched -> keeps its own code (resolving the acting sender is a read)
usage/401/403/404/409/422/429, and 4xx generally
-> keeps its own code; HEY refused before it acted
transport failure -> ambiguous
5xx (500/502/503/504)
-> ambiguous, which is the SDK's own mutation contract: it does
not retry a POST on 5xx for exactly this reason
response unreadable or past the cap after a 2xx
-> ambiguous, and the strongest form of it — HEY said yes and the
answer naming what it made is gone
context cancelled or timed out, or anything unrecognised
-> ambiguous
Codes are read rather than sentences, and the status is consulted second because the SDK
does not put one on every refusal it builds: ErrAuth and ErrNotFound carry none. An
oversized *error* body keeps its status, so a 422 too large to read is still a 422.
errNotDispatched marks the pre-send sender resolution, which is a read; the original
error travels inside the wrapper so errors.As still finds the *hey.Error underneath.
This is wired to the send call sites and nowhere else. A read that cannot reach HEY is
still a read failure — reading again costs nothing and delivers nothing — and
TestAFailedReadIsNeverAnAmbiguousSend is what holds that line.
Nothing here retries. The SDK does not retry a POST either, except once after a 401 it
refreshed credentials for, and a 401 is HEY declining to act at all.
The reviewer's probe is committed for both send paths: the server consumes the whole
request body, hijacks the connection and closes it, and the tests assert the server saw
exactly one POST — no retry — alongside the ambiguous outcome.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Exact sender verification, Markdown detection, URL-path parsing, and retention budgeting contain correctness gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds known-ID delivery reconciliation for compose operations and exposes message verification metadata.
Changes:
- Adds
compose --verifiablewith draft/read/send reconciliation. - Adds compose handles, verification envelopes, and ambiguous-outcome handling.
- Exposes thread addressing metadata with expanded tests and documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
.surface |
Records the new flag. |
README.md |
Adds a verifiable-compose example. |
docs/cli.md |
Documents reconciliation semantics. |
skills/hey/SKILL.md |
Adds agent guidance. |
tests/smoke/compose_test.go |
Expands compose smoke assertions. |
internal/apierr/apierr.go |
Adds ambiguous-outcome errors. |
internal/threadload/threadload.go |
Retains message addressing data. |
internal/threadload/threadload_test.go |
Updates retention-budget tests. |
internal/cmd/accounts_test.go |
Updates compose fixture response. |
internal/cmd/addressed.go |
Models bounded recipient envelopes. |
internal/cmd/addressed_test.go |
Tests BCC disclosure semantics. |
internal/cmd/attachments_test.go |
Supports compose readback fixtures. |
internal/cmd/compose.go |
Integrates verifiable compose and results. |
internal/cmd/compose_dispatch_test.go |
Tests ambiguous dispatch outcomes. |
internal/cmd/compose_handle.go |
Extracts created-message handles. |
internal/cmd/compose_handle_test.go |
Tests response-handle parsing. |
internal/cmd/compose_send.go |
Preserves send responses and classifies failures. |
internal/cmd/compose_verifiable.go |
Implements known-ID reconciliation. |
internal/cmd/compose_verifiable_test.go |
Tests the verifiable lifecycle. |
internal/cmd/compose_verify.go |
Builds readback verification results. |
internal/cmd/compose_verify_test.go |
Tests compose verification behavior. |
internal/cmd/root.go |
Selects no-refresh authentication mode. |
internal/cmd/root_test.go |
Tests migration exclusions. |
internal/cmd/sdk.go |
Adds authentication without replay. |
internal/cmd/thread_envelope_test.go |
Tests thread addressing output. |
internal/cmd/thread_reply_test.go |
Adds reply readback fixtures. |
internal/cmd/topic.go |
Exposes message envelopes in thread JSON. |
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+266
to
+268
| case xhtml.StartTagToken, xhtml.EndTagToken, xhtml.SelfClosingTagToken, | ||
| xhtml.CommentToken, xhtml.DoctypeToken: | ||
| return true |
Comment on lines
+118
to
+119
| setID(&handle.MessageID, idAfter(location, "/messages/")) | ||
| setID(&handle.TopicID, mail.TopicIDIn(location)) |
Comment on lines
+105
to
+112
| exact := draft.Id == draftID && | ||
| draft.Subject == sent.Subject && | ||
| bodyDigest(canonicalBody(draft.Content)) == bodyDigest(canonicalBody(sent.Content)) && | ||
| !recipients.Truncated && | ||
| sameAddresses(sent.To, recipients.To) && | ||
| sameAddresses(sent.CC, recipients.CC) && | ||
| recipients.BCCDisclosed && | ||
| sameAddresses(sent.BCC, recipients.BCC) |
|
|
||
| if message := loaded.Message; message != nil { | ||
| subject = message.Subject | ||
| sender = senderOf(message) |
Comment on lines
+421
to
+422
| Sender: message.Sender, | ||
| Addressed: message.Addressed, |
Comment on lines
69
to
+70
| hey compose --to alice@example.com --subject "Lunch?" -m "Thursday at noon?" | ||
| hey compose --to alice@example.com --subject "Customer update" -m "Done." --verifiable |
Comment on lines
+49
to
50
| "-m", body, | ||
| "--json", |
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
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.
Summary
Adds a reconcilable compose mode for callers that must not guess whether an email was delivered.
hey compose --verifiablesaves a draft first to obtain a stable message IDThe branch also makes ordinary compose results expose their message/topic handle, preserves BCC field-disclosure semantics, and adds the same sender/addressing envelope to JSON thread reads.
Safety behavior
--verifiablerejects drafts, replies, attachments, raw HTML, and Action Text attachment markup before a write. It uses normal credential authentication but deliberately disables the SDK's post-401 credential-refresh replay for this command only. Other commands retain the existing refresh behavior.Verification
go test ./...make fmt-check vet lint tidy-check check-surface check-release-lockstep buildgolangci-lint: 0 issuesSummary by cubic
Adds
hey compose --verifiable, which proves an email was delivered instead of trusting a 2xx response. The command saves a draft first to get a stable message ID, reads it back to confirm sender, subject, body, To, CC, and disclosed BCC match, then sends exactly once and verifies the delivered message by ID.New Features
--verifiablerejects drafts, replies, attachments, raw HTML, and Action Text markup before writing.hey composenow reports the created message/topic handle and app URL in JSON output.Bug Fixes
ambiguous(exit 8) instead ofnetwork, so callers don't retry and risk double delivery.bcc_disclosednow means HEY served the BCC line, not that anyone was on it, so an empty served line is distinguishable from a withheld one.hey thread read --jsonnow includes the message's sender and addressing envelope, bounded per message.Written for commit 3397183. Summary will update on new commits.