From b6d6f2ea20e9e1eb4b33946441217a1454197a4b Mon Sep 17 00:00:00 2001 From: jonata97 Date: Wed, 2 Sep 2026 14:26:16 -0300 Subject: [PATCH 1/5] feat(compose): answer a handle and verify the message a send created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- docs/cli.md | 4 + internal/apierr/apierr.go | 9 + internal/cmd/accounts_test.go | 3 + internal/cmd/addressed.go | 63 ++++ internal/cmd/attachments_test.go | 11 + internal/cmd/compose.go | 59 ++- internal/cmd/compose_handle.go | 153 ++++++++ internal/cmd/compose_handle_test.go | 158 ++++++++ internal/cmd/compose_send.go | 67 ++++ internal/cmd/compose_verify.go | 259 +++++++++++++ internal/cmd/compose_verify_test.go | 495 +++++++++++++++++++++++++ internal/cmd/thread_envelope_test.go | 190 ++++++++++ internal/cmd/thread_reply_test.go | 21 ++ internal/cmd/topic.go | 43 ++- internal/threadload/threadload.go | 36 +- internal/threadload/threadload_test.go | 15 +- tests/smoke/compose_test.go | 109 +++++- 17 files changed, 1660 insertions(+), 35 deletions(-) create mode 100644 internal/cmd/addressed.go create mode 100644 internal/cmd/compose_handle.go create mode 100644 internal/cmd/compose_handle_test.go create mode 100644 internal/cmd/compose_send.go create mode 100644 internal/cmd/compose_verify.go create mode 100644 internal/cmd/compose_verify_test.go create mode 100644 internal/cmd/thread_envelope_test.go diff --git a/docs/cli.md b/docs/cli.md index f860572e..086debbd 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -263,6 +263,10 @@ Writing is Markdown too, everywhere text goes in: `-m`, `--content`, `--note`, p Drafts are the review-before-send lane: `hey compose --draft` (and `hey reply --draft`) saves instead of sending — recipients optional on a draft — and answers the draft's ID. `hey draft show` reads it back with the body as Markdown, `hey draft edit` revises it (each flag replaces its field; what is not flagged is kept, by reading the draft and resending the whole of it, since a revision is not a patch on HEY's side), `hey draft send` delivers through HEY's undo window, and `hey draft delete` trashes it. Scheduling a delivery is done in a HEY app for now — the API cannot yet name an exact instant — and a schedule set there survives CLI edits untouched. A draft prepared here is reviewed and sent from any HEY app, which is the workflow this is for: an agent writes, a person decides. +A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` records whether HEY served the delivered message's BCC line. A send HEY accepted but named no message for is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. `--styled` output is unchanged: one line, `Message sent.` + +`hey thread read --json` carries the same envelope per entry, off the same message record: `subject`, `sender` (the identity the message went out as, next to `creator`, who wrote it) and `addressed` with `to`, `cc`, `bcc` and `bcc_disclosed`. Recipient lists are read from what HEY served, never inferred from a position or from the body, and are cut at a hundred addresses per line with `truncated` saying so. `--count` and `--ids-only` read no messages, so they carry no envelope rather than an invented one. + `hey share ` gets a sharing link for a thread. Anyone with the link can see the entire thread and future emails or replies sent to it. `hey unshare ` turns off the sharing link. Search accepts free text plus `--required`, `--any`, `--none`, `--exact`, `--from`, `--to`, `--subject`, `--date`, `--in`, `--label`, and `--attachment`. `--in`, `--date`, `--label` and `--attachment` take one of the values `hey search filters` lists — the attachment kinds are `any`, `images`, `pdfs`, `calendar_invites`, `documents`, `spreadsheets`, `presentations`, `media` and `zip_files`, so it is `--attachment pdfs` rather than `pdf`, and an unrecognized `--in`, `--date` or `--attachment` is refused with the values it accepts before anything is sent. Use `--page` for one page or `--all` to fetch up to 100 pages; capped searches report the next page for continuation. Search results include `topic_id` for reading the thread and the matching message summaries. Results with an active box item also include `id` for organization actions. diff --git a/internal/apierr/apierr.go b/internal/apierr/apierr.go index a653c2a2..f920abbd 100644 --- a/internal/apierr/apierr.go +++ b/internal/apierr/apierr.go @@ -143,6 +143,15 @@ func ErrAmbiguous(resource string, matches []string) *Error { } } +// ErrAmbiguousOutcome is a write the server accepted and then named nothing for. It is +// neither a success nor a failure: the change may well have happened, so a caller must +// go and look rather than either reporting it done or trying again. It is a code of its +// own — not api, not network — because those two both read as "it did not happen", and +// this one does not. +func ErrAmbiguousOutcome(msg, hint string) *Error { + return &Error{Code: CodeAmbiguous, Message: msg, Hint: hint} +} + func AsError(err error) *Error { var e *Error if errors.As(err, &e) { diff --git a/internal/cmd/accounts_test.go b/internal/cmd/accounts_test.go index 907c004d..875b3e3b 100644 --- a/internal/cmd/accounts_test.go +++ b/internal/cmd/accounts_test.go @@ -159,6 +159,9 @@ func TestSelectedAccountUsesMatchingSenderAndUser(t *testing.T) { } _ = json.NewDecoder(r.Body).Decode(&body) actingSenderID = body.ActingSenderID + // HEY names the entry it created in Location, which is what compose reads + // back; the readback itself 404s here, and an unverified send still stands. + w.Header().Set("Location", "https://app.hey.com/messages/9101") w.WriteHeader(http.StatusCreated) case "/contacts.json": contactAccount = r.URL.Query().Get("filtered_account_id") diff --git a/internal/cmd/addressed.go b/internal/cmd/addressed.go new file mode 100644 index 00000000..a7a6c4f7 --- /dev/null +++ b/internal/cmd/addressed.go @@ -0,0 +1,63 @@ +package cmd + +import ( + "github.com/basecamp/hey-sdk/go/pkg/generated" +) + +// maxRetainedRecipients is how many addresses of one kind are kept from a message HEY +// served. A message addressed to a mailing list carries as many recipients as somebody +// else decided to put on it, and every one of them is a name and an address this +// program would otherwise hold and print; the bound is what keeps a read of one message +// from retaining an address book. It is well past any list a person writes by hand. +const maxRetainedRecipients = 100 + +// addressedEnvelope is who a message reached, as HEY served it back: the To, CC and BCC +// lines as plain addresses. It is what lets a caller prove a message went where it was +// meant to and nowhere else, so it is read off the message HEY answered with rather +// than inferred from a position in a list or found in the body. +// +// BCCDisclosed is the honest part. HEY does not distinguish "there was no BCC" from "we +// are not telling you the BCC" — both arrive as no blindcopied recipients at all — so +// BCC being empty is never proof that nobody was blind-copied. BCCDisclosed is true only +// when HEY actually served addresses, and a caller that must not accept an unexpected +// recipient reads it before treating an empty BCC as settled. +type addressedEnvelope struct { + To []string `json:"to"` + CC []string `json:"cc"` + BCC []string `json:"bcc"` + BCCDisclosed bool `json:"bcc_disclosed"` + // Truncated says a list was longer than maxRetainedRecipients and was cut to it. + Truncated bool `json:"truncated,omitempty"` +} + +// addressedFrom describes HEY's addressing in the CLI's shape, within the bound. +func addressedFrom(addressed generated.Addressed) addressedEnvelope { + to, toCut := boundedEmails(addressed.Directly) + cc, ccCut := boundedEmails(addressed.Copied) + bcc, bccCut := boundedEmails(addressed.Blindcopied) + return addressedEnvelope{ + To: to, + CC: cc, + BCC: bcc, + BCCDisclosed: len(bcc) > 0, + Truncated: toCut || ccCut || bccCut, + } +} + +// boundedEmails answers the contacts' addresses, dropping blanks and stopping at +// maxRetainedRecipients. The addresses are HEY's own, so they are carried verbatim; +// whatever prints one sanitizes it there, as every other read of somebody else's text +// does. +func boundedEmails(contacts []generated.Contact) (emails []string, truncated bool) { + emails = make([]string, 0, min(len(contacts), maxRetainedRecipients)) + for _, contact := range contacts { + if contact.EmailAddress == "" { + continue + } + if len(emails) == maxRetainedRecipients { + return emails, true + } + emails = append(emails, contact.EmailAddress) + } + return emails, false +} diff --git a/internal/cmd/attachments_test.go b/internal/cmd/attachments_test.go index 40fb3913..f987ab29 100644 --- a/internal/cmd/attachments_test.go +++ b/internal/cmd/attachments_test.go @@ -104,6 +104,7 @@ func attachmentServer(t *testing.T) (*httptest.Server, *attachmentServerState) { state.sentContents = append(state.sentContents, body.Message.Content) state.events = append(state.events, "send") state.mu.Unlock() + w.Header().Set("Location", "https://app.hey.com/messages/9101") w.WriteHeader(http.StatusCreated) _, _ = w.Write([]byte(`{}`)) case r.Method == http.MethodPost && r.URL.Path == "/messages.json": @@ -117,8 +118,18 @@ func attachmentServer(t *testing.T) (*httptest.Server, *attachmentServerState) { state.sentContents = append(state.sentContents, body.Message.Content) state.events = append(state.events, "send") state.mu.Unlock() + w.Header().Set("Location", "https://app.hey.com/messages/9101") w.WriteHeader(http.StatusCreated) _, _ = w.Write([]byte(`{}`)) + case r.Method == http.MethodGet && r.URL.Path == "/messages/9101.json": + state.mu.Lock() + content := "" + if len(state.sentContents) > 0 { + content = state.sentContents[len(state.sentContents)-1] + } + state.mu.Unlock() + payload, _ := json.Marshal(map[string]any{"id": 9101, "content": content}) + _, _ = w.Write(payload) default: t.Logf("unhandled attachment test request: %s %s", r.Method, r.URL.RequestURI()) http.Error(w, "not found", http.StatusNotFound) diff --git a/internal/cmd/compose.go b/internal/cmd/compose.go index 3b1f75d6..edca1178 100644 --- a/internal/cmd/compose.go +++ b/internal/cmd/compose.go @@ -98,6 +98,15 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { ctx := cmd.Context() + // A send answers where the message went, so the response HEY gives is kept rather + // than reduced to an error: sendClient, response and sent below are what + // composeHandle and verifyComposedMessage work from. + var ( + sendClient = sdk + response *hey.Response + sent composeSent + ) + if c.threadID != "" { topicID, parseErr := strconv.ParseInt(c.threadID, 10, 64) if parseErr != nil { @@ -120,9 +129,19 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { } return writeDraftSaved(cmd, draftID, len(c.attachments)) } - if err := replySDK.Entries().CreateReply(ctx, target.EntryID, target.ActingSenderID, target.Subject, messageWithAttachments, - target.Addressed.To, target.Addressed.CC, target.Addressed.BCC); err != nil { - return apierr.FromSDK(err) + if len(target.Addressed.To)+len(target.Addressed.CC)+len(target.Addressed.BCC) == 0 { + return apierr.ErrUsage("a reply needs at least one recipient (to, cc or bcc); HEY saves an unaddressed reply as a draft") + } + sendClient = replySDK + sent = composeSent{ + Subject: target.Subject, Content: messageWithAttachments, + To: target.Addressed.To, CC: target.Addressed.CC, BCC: target.Addressed.BCC, + } + var sendErr error + response, sendErr = sendReply(ctx, replySDK, target.EntryID, target.ActingSenderID, + sent.Subject, sent.Content, sent.To, sent.CC, sent.BCC) + if sendErr != nil { + return apierr.FromSDK(sendErr) } } else { to := parseAddresses(c.to) @@ -145,12 +164,40 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { } return writeDraftSaved(cmd, draftID, len(c.attachments)) } - if err := sdk.Messages().Create(ctx, c.subject, messageWithAttachments, to, cc, bcc); err != nil { - return apierr.FromSDK(err) + sent = composeSent{Subject: c.subject, Content: messageWithAttachments, To: to, CC: cc, BCC: bcc} + var sendErr error + response, sendErr = sendMessage(ctx, sdk, sent.Subject, sent.Content, sent.To, sent.CC, sent.BCC) + if sendErr != nil { + return apierr.FromSDK(sendErr) } } - return writeMutation(cmd, sentWithAttachmentsSummary("Message sent", len(c.attachments)), nil) + // HEY accepted the request. Which message it made is a separate question, and one a + // caller that has to prove what it sent cannot do without: a send that names nothing + // is reported as ambiguous rather than as a success, so nobody reads "sent" off a + // response there is no way back from — and nobody retries a send that may already + // have gone out. + handle, handleErr := handleFromResponse(response.StatusCode, response.Headers, response.Data) + if handleErr != nil { + return apierr.ErrAmbiguousOutcome( + fmt.Sprintf("the message may have been sent, but the response named no message to read back: %v", handleErr), + "Check the thread before sending again — a retry may deliver it twice.") + } + + verification, readback := verifyComposedMessage(ctx, sendClient, handle.MessageID, sent) + result := composeResultFor(handle, verification, readback) + + summary := sentWithAttachmentsSummary("Message sent", len(c.attachments)) + if result.TopicID != 0 { + return writeMutation(cmd, summary, result, output.WithBreadcrumbs( + output.Breadcrumb{ + Action: "read", + Command: fmt.Sprintf("hey thread read %d", result.TopicID), + Description: "Read the thread this message landed in", + }, + )) + } + return writeMutation(cmd, summary, result) } // writeDraftSaved confirms a saved draft, naming the id every draft verb takes. diff --git a/internal/cmd/compose_handle.go b/internal/cmd/compose_handle.go new file mode 100644 index 00000000..270e6bdc --- /dev/null +++ b/internal/cmd/compose_handle.go @@ -0,0 +1,153 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "net/http" + "strconv" + "strings" + + "github.com/basecamp/hey-cli/internal/mail" +) + +// composeHandle names what a send created. +// +// Without one, `hey compose` can report that HEY accepted a request and nothing more, +// and "the server said 2xx" is not evidence that a particular message exists. Anything +// that has to prove what it sent — an automated sender, an audit, a retry that must not +// deliver twice — needs an identifier it can read back, and this endpoint carries no +// idempotency key to fall back on. +type composeHandle struct { + MessageID int64 + TopicID int64 + AppURL string +} + +// usable reports whether the handle names something that can be read back. A message id +// is what the readback wants; a topic id alone still names the thread the message landed +// in, which is enough for a caller to go and find it. +func (h composeHandle) usable() bool { + return h.MessageID != 0 || h.TopicID != 0 +} + +// composeResponseBody is every field of a send's response this program will read. It is +// a closed list on purpose: 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 that is told "sent" stops looking. A body that does not +// unmarshal into this shape — HTML, an array, an id served as a string — is left to the +// Location header rather than guessed at. +type composeResponseBody struct { + ID int64 `json:"id"` + MessageID int64 `json:"message_id"` + TopicID int64 `json:"topic_id"` + AppURL string `json:"app_url"` + Message *struct { + ID int64 `json:"id"` + TopicID int64 `json:"topic_id"` + AppURL string `json:"app_url"` + } `json:"message"` +} + +// handleFromResponse mines a send's response for a handle, from the two places HEY is +// known to put one: +// +// 1. the Location header, which is how a saved draft answers already +// (204 No Content, Location: …/messages/{entry_id}) — see the SDK's +// draftEntryIDFromLocation, which the same controllers serve; +// 2. a JSON body naming the message (`id`, `message_id`, `message.id`) or the thread +// (`topic_id`, `app_url`). +// +// Both are consulted whichever answered first: a mutation may serve an empty body and a +// header, and a body that named only the thread is improved by a header that names the +// message. +// +// A response that names neither is an error. The request was accepted, so a message may +// well exist — that is exactly why this refuses rather than reporting a plain success. +func handleFromResponse(status int, headers http.Header, body []byte) (composeHandle, error) { + if status < 200 || status > 299 { + return composeHandle{}, fmt.Errorf("the send answered HTTP %d", status) + } + + var handle composeHandle + mergeResponseBody(&handle, body) + mergeLocation(&handle, headers) + + if !handle.usable() { + return composeHandle{}, fmt.Errorf( + "HTTP %d named no message id, no thread id and no Location", status) + } + return handle, nil +} + +// mergeResponseBody fills in whatever the body names, and stays silent otherwise: the +// Location header may still carry the handle, and handleFromResponse decides whether +// what was collected between them is enough. +func mergeResponseBody(handle *composeHandle, body []byte) { + if len(strings.TrimSpace(string(body))) == 0 { + return + } + var parsed composeResponseBody + if err := json.Unmarshal(body, &parsed); err != nil { + return + } + + // The endpoint creates a message, so a bare `id` is that message's. + setID(&handle.MessageID, parsed.ID, parsed.MessageID) + setID(&handle.TopicID, parsed.TopicID) + if parsed.Message != nil { + setID(&handle.MessageID, parsed.Message.ID) + setID(&handle.TopicID, parsed.Message.TopicID) + if handle.AppURL == "" { + handle.AppURL = parsed.Message.AppURL + } + } + if handle.AppURL == "" { + handle.AppURL = parsed.AppURL + } + setID(&handle.TopicID, mail.TopicIDIn(handle.AppURL)) +} + +// mergeLocation reads the ids out of the Location header's path. The header is HEY's +// own, and only its path is read, so there is no origin to get wrong here: a URL that +// names neither a message nor a topic simply contributes nothing. +func mergeLocation(handle *composeHandle, headers http.Header) { + location := headers.Get("Location") + if location == "" { + return + } + setID(&handle.MessageID, idAfter(location, "/messages/")) + setID(&handle.TopicID, mail.TopicIDIn(location)) +} + +// setID keeps the first positive id it is offered, so a body that already named the +// message is not overwritten by a header that names it again. +func setID(target *int64, candidates ...int64) { + if *target > 0 { + return + } + for _, candidate := range candidates { + if candidate > 0 { + *target = candidate + return + } + } +} + +// idAfter reads the number following the last occurrence of segment in a URL path, +// stopping where the segment does. It answers zero for anything that is not a number, +// which is what keeps a `…/messages/new` out of a handle. +func idAfter(rawURL, segment string) int64 { + marker := strings.LastIndex(rawURL, segment) + if marker < 0 { + return 0 + } + rest := rawURL[marker+len(segment):] + if end := strings.IndexAny(rest, "/?#."); end >= 0 { + rest = rest[:end] + } + id, err := strconv.ParseInt(rest, 10, 64) + if err != nil || id <= 0 { + return 0 + } + return id +} diff --git a/internal/cmd/compose_handle_test.go b/internal/cmd/compose_handle_test.go new file mode 100644 index 00000000..f51c1870 --- /dev/null +++ b/internal/cmd/compose_handle_test.go @@ -0,0 +1,158 @@ +package cmd + +import ( + "net/http" + "testing" +) + +// handleFromResponse only accepts shapes HEY is known to answer with. Every accepted +// shape has a case here; everything else is an error rather than a guess, because a +// lenient parser's failure mode is reporting a message as sent with nothing that can +// show which one it was. +func TestHandleFromResponseReadsTheShapesHEYAnswersWith(t *testing.T) { + tests := []struct { + name string + status int + headers http.Header + body string + wantMsg int64 + wantTopic int64 + wantURL string + }{ + { + name: "a Location naming the entry, which is how a draft save answers", + status: http.StatusNoContent, + headers: locationHeader("https://app.hey.com/messages/9101"), + body: "null", + wantMsg: 9101, + }, + { + name: "a relative Location", + status: http.StatusCreated, + headers: locationHeader("/messages/9101"), + wantMsg: 9101, + }, + { + name: "a Location naming the thread", + status: http.StatusCreated, + headers: locationHeader("https://app.hey.com/topics/7742"), + wantTopic: 7742, + }, + { + name: "a Location naming both", + status: http.StatusCreated, + headers: locationHeader("https://app.hey.com/topics/7742/messages/9101"), + wantMsg: 9101, + wantTopic: 7742, + }, + { + name: "a body that is the created message", + status: http.StatusCreated, + body: `{"id": 9101, "subject": "Inovo Customer Update"}`, + wantMsg: 9101, + }, + { + name: "a body naming the message explicitly", + status: http.StatusCreated, + body: `{"message_id": 9101}`, + wantMsg: 9101, + }, + { + name: "a body wrapping the message", + status: http.StatusCreated, + body: `{"message": {"id": 9101, "topic_id": 7742}}`, + wantMsg: 9101, + wantTopic: 7742, + }, + { + name: "a body naming only the thread", + status: http.StatusOK, + body: `{"topic_id": 7742, "app_url": "https://app.hey.com/topics/7742"}`, + wantTopic: 7742, + wantURL: "https://app.hey.com/topics/7742", + }, + { + name: "an app_url that names the thread the id field did not", + status: http.StatusOK, + body: `{"id": 9101, "app_url": "https://app.hey.com/topics/7742"}`, + wantMsg: 9101, + wantTopic: 7742, + wantURL: "https://app.hey.com/topics/7742", + }, + { + name: "the header improves a body that named only the thread", + status: http.StatusCreated, + headers: locationHeader("https://app.hey.com/messages/9101"), + body: `{"topic_id": 7742}`, + wantMsg: 9101, wantTopic: 7742, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handle, err := handleFromResponse(tt.status, tt.headers, []byte(tt.body)) + if err != nil { + t.Fatalf("handleFromResponse: %v", err) + } + if handle.MessageID != tt.wantMsg { + t.Errorf("message id = %d, want %d", handle.MessageID, tt.wantMsg) + } + if handle.TopicID != tt.wantTopic { + t.Errorf("topic id = %d, want %d", handle.TopicID, tt.wantTopic) + } + if handle.AppURL != tt.wantURL { + t.Errorf("app url = %q, want %q", handle.AppURL, tt.wantURL) + } + }) + } +} + +// Anything that does not name a message or a thread is refused. The request may well +// have been accepted — that is exactly why it is refused rather than reported as a +// plain success: a caller must not read "sent" off a response nothing can be read back +// from. +func TestHandleFromResponseRefusesWhatItCannotReadBack(t *testing.T) { + tests := []struct { + name string + status int + headers http.Header + body string + }{ + {name: "no body and no header", status: http.StatusNoContent, body: ""}, + {name: "the null the SDK maps 204 to", status: http.StatusNoContent, body: "null"}, + {name: "an empty object", status: http.StatusCreated, body: "{}"}, + {name: "a shape nothing here knows", status: http.StatusCreated, body: `{"status": "ok"}`}, + {name: "an array", status: http.StatusCreated, body: `[{"id": 9101}]`}, + {name: "HTML", status: http.StatusOK, body: `Sent`}, + {name: "a zero id", status: http.StatusCreated, body: `{"id": 0}`}, + {name: "a negative id", status: http.StatusCreated, body: `{"id": -3}`}, + {name: "an id that is not a number", status: http.StatusCreated, body: `{"id": "9101"}`}, + { + name: "a Location naming neither", + status: http.StatusCreated, + headers: locationHeader("https://app.hey.com/imbox"), + }, + { + name: "a Location whose id is not a number", + status: http.StatusCreated, + headers: locationHeader("https://app.hey.com/messages/new"), + }, + { + name: "a status outside 2xx, header or not", + status: http.StatusInternalServerError, + headers: locationHeader("https://app.hey.com/messages/9101"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if _, err := handleFromResponse(tt.status, tt.headers, []byte(tt.body)); err == nil { + t.Fatal("want an error, got a handle") + } + }) + } +} + +func locationHeader(location string) http.Header { + return http.Header{"Location": []string{location}} +} diff --git a/internal/cmd/compose_send.go b/internal/cmd/compose_send.go new file mode 100644 index 00000000..8df8f725 --- /dev/null +++ b/internal/cmd/compose_send.go @@ -0,0 +1,67 @@ +package cmd + +import ( + "context" + "fmt" + + "github.com/basecamp/hey-sdk/go/pkg/generated" + hey "github.com/basecamp/hey-sdk/go/pkg/hey" +) + +// A send is posted here rather than through MessagesService.Create and +// EntriesService.CreateReply because both of those return an error and nothing else: +// the response — its status, its Location header, its body — is dropped on the floor, +// and with it the only thing that names the message that was just created. A caller +// that has to prove what it sent cannot work from "no error"; see composeHandle. +// +// The request bodies are the SDK's own generated wire types, so the shape of the +// request stays defined in one place and this is a response-preserving wrapper rather +// than a second definition of the API. The transport is the SDK's too — the same auth, +// the same account scope, the same response cap — and POST is never retried there, so +// nothing here can deliver a message twice. +// +// The durable home for this is a CreateWithResult on the SDK's own services. Until +// there is one, keep these two functions the only place hey-cli builds a send by hand. + +// sendMessage starts a new thread and answers the response HEY gave. +func sendMessage(ctx context.Context, client *hey.Client, subject, content string, to, cc, bcc []string) (*hey.Response, error) { + senderID, err := actingSenderID(ctx, client, 0) + if err != nil { + return nil, err + } + body := generated.CreateMessageRequestContent{ + ActingSenderId: senderID, + Message: generated.MessagePayload{Subject: subject, Content: content}, + Entry: &generated.MessageEntryPayload{ + Addressed: &generated.MessageAddressed{Directly: to, Copied: cc, Blindcopied: bcc}, + }, + } + return client.PostMutation(ctx, "/messages.json", body) +} + +// sendReply answers an entry and answers the response HEY gave. The acting sender is +// the one the thread resolved to, since a thread on a shared or alternate address does +// not go out as the account default. +func sendReply(ctx context.Context, client *hey.Client, entryID, actingSender int64, subject, content string, to, cc, bcc []string) (*hey.Response, error) { + senderID, err := actingSenderID(ctx, client, actingSender) + if err != nil { + return nil, err + } + body := generated.CreateReplyRequestContent{ + ActingSenderId: senderID, + Message: generated.ReplyMessagePayload{Subject: subject, Content: content}, + Entry: &generated.MessageEntryPayload{ + Addressed: &generated.MessageAddressed{Directly: to, Copied: cc, Blindcopied: bcc}, + }, + } + return client.PostMutation(ctx, fmt.Sprintf("/entries/%d/replies.json", entryID), body) +} + +// actingSenderID resolves the identity a send goes out as, the way the SDK's services +// do: a sender the caller chose stands, and zero falls back to the account's default. +func actingSenderID(ctx context.Context, client *hey.Client, chosen int64) (int64, error) { + if chosen != 0 { + return chosen, nil + } + return client.DefaultSenderID(ctx) +} diff --git a/internal/cmd/compose_verify.go b/internal/cmd/compose_verify.go new file mode 100644 index 00000000..2d429c08 --- /dev/null +++ b/internal/cmd/compose_verify.go @@ -0,0 +1,259 @@ +package cmd + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "strings" + + "github.com/basecamp/hey-sdk/go/pkg/generated" + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + + "github.com/basecamp/hey-cli/internal/htmlutil" + "github.com/basecamp/hey-cli/internal/mail" +) + +// maxVerifiedBodyBytes is the most Markdown a verification carries inline. A body past +// it is left out and compared by digest instead: half a body invites a comparison that +// looks like it succeeded, and the digest — which is over the whole of it either way — +// answers the only question a verifier is really asking. +const maxVerifiedBodyBytes = 64 << 10 + +// The statuses a verification reports. They are three different instructions to a +// caller, which is the whole reason they are separate words. +const ( + // verificationVerified: the message was read back and everything comparable about + // it matches what was asked for. + verificationVerified = "verified" + // verificationMismatch: the message was read back and something differs. The + // message exists — this is never a reason to send again. + verificationMismatch = "mismatch" + // verificationUnverified: the message could not be read back. It may well exist, + // so this is not a failed send either; Reason says what stopped the read. + verificationUnverified = "unverified" +) + +// composeSent is what the caller asked to be sent, kept so the readback has something +// to be compared against. +type composeSent struct { + Subject string + // Content is the Trix HTML that actually went on the wire, attachments included. + Content string + To []string + CC []string + BCC []string +} + +// composeVerification is what reading the message back showed. It is deliberately +// bounded: the recipient lists stop at maxRetainedRecipients and the body at +// maxVerifiedBodyBytes, so a verification is a fixed-size statement about a message +// rather than a copy of whatever the server chose to serve. +type composeVerification struct { + Status string `json:"status"` + Method string `json:"method"` + // Reason is set only when Status is unverified, and says what stopped the read. + Reason string `json:"reason,omitempty"` + + Subject string `json:"subject,omitempty"` + Sender *threadContact `json:"sender,omitempty"` + Recipients *addressedEnvelope `json:"recipients,omitempty"` + + // BodyMarkdown is the stored body as canonical Markdown — the same conversion + // `hey thread read` publishes, so the two agree byte for byte. + BodyMarkdown htmlutil.Markdown `json:"body_markdown,omitzero"` + // BodyTruncated says the body was past maxVerifiedBodyBytes and was left out; + // BodyMarkdownSHA256 still covers the whole of it. + BodyTruncated bool `json:"body_truncated,omitempty"` + // BodyMarkdownSHA256 is over the canonical Markdown, not over HEY's HTML: HTML is + // the server's to reformat, Markdown is what both ends can agree on. + BodyMarkdownSHA256 string `json:"body_markdown_sha256,omitempty"` + + MatchesSent *composeMatches `json:"matches_sent,omitempty"` +} + +// composeMatches compares what came back with what was sent, one answer per thing a +// sender cares about. +type composeMatches struct { + Subject bool `json:"subject"` + Body bool `json:"body"` + // Recipients holds when the To and CC lines are exactly the ones asked for and + // every BCC address served back was asked for too. An undisclosed BCC does not + // break it — HEY commonly serves none — but a recipient nobody asked for does. + Recipients bool `json:"recipients"` +} + +// verifyComposedMessage reads the created message back and says what that showed. +// +// It is the readback half of the send contract: `hey compose` reports what HEY stored, +// not what it was handed, so a caller can prove the identity of the message it just +// created instead of trusting a status code. The read is `Messages().Get` — the same +// request `hey thread read` makes per entry — so what it reports and what a later +// thread read reports are the same conversion of the same record. +// +// It also answers the message itself, which is where a topic id and an app URL come +// from when the send's own response named neither. +func verifyComposedMessage(ctx context.Context, client *hey.Client, messageID int64, sent composeSent) (composeVerification, *generated.Message) { + if messageID == 0 { + return composeVerification{ + Status: verificationUnverified, + Method: "none", + Reason: "the send named a thread but no message, so there is no message to read back", + }, nil + } + + message, err := client.Messages().Get(ctx, messageID) + if err != nil { + return composeVerification{ + Status: verificationUnverified, + Method: "message_read", + Reason: "the message could not be read back", + }, nil + } + if message == nil { + return composeVerification{ + Status: verificationUnverified, + Method: "message_read", + Reason: "the message read back was empty", + }, nil + } + + recipients := addressedFrom(message.Addressed) + body, truncated, digest := verifiedBody(message.Content) + matches := composeMatches{ + Subject: message.Subject == sent.Subject, + Body: digest == bodyDigest(canonicalBody(sent.Content)), + Recipients: recipientsMatch(sent, recipients), + } + + status := verificationVerified + if !matches.Subject || !matches.Body || !matches.Recipients { + status = verificationMismatch + } + + return composeVerification{ + Status: status, + Method: "message_read", + Subject: message.Subject, + Sender: senderOf(message), + Recipients: &recipients, + + BodyMarkdown: body, + BodyTruncated: truncated, + BodyMarkdownSHA256: digest, + + MatchesSent: &matches, + }, message +} + +// senderOf is the identity the message went out as, falling back to whoever wrote it +// when HEY names no separate sender. +func senderOf(message *generated.Message) *threadContact { + contact := message.Sender + if contact.EmailAddress == "" && contact.Name == "" { + contact = message.Creator + } + if contact.EmailAddress == "" && contact.Name == "" && contact.Id == 0 { + return nil + } + return &threadContact{ID: contact.Id, Name: contact.Name, EmailAddress: contact.EmailAddress} +} + +// verifiedBody converts a stored body to canonical Markdown, within the inline bound. +// The digest is over the whole of it whether or not the Markdown itself is carried, so +// a body too large to publish can still be compared. +func verifiedBody(content string) (body htmlutil.Markdown, truncated bool, digest string) { + markdown := htmlutil.ToMarkdown(content) + digest = bodyDigest(markdown.String()) + if len(markdown.String()) > maxVerifiedBodyBytes { + return htmlutil.Markdown{}, true, digest + } + return markdown, false, digest +} + +// canonicalBody is the sent body in the same form the readback is measured in, so the +// two are compared as Markdown rather than as HTML. HEY is free to reformat the markup +// it stores; what it may not do is change what the message says. +func canonicalBody(content string) string { + return htmlutil.ToMarkdown(content).String() +} + +// bodyDigest is SHA-256 over the canonical Markdown, hex-encoded. +func bodyDigest(markdown string) string { + sum := sha256.Sum256([]byte(markdown)) + return hex.EncodeToString(sum[:]) +} + +// recipientsMatch reports whether the message reached exactly who it was meant to. +// +// To and CC must be the sets that were asked for, in any order. BCC is one-sided: HEY +// serves no blindcopied line back for a message it delivered, and an empty list is not +// proof that nobody was blind-copied, so a BCC that came back must have been asked for +// but one that did not come back costs nothing. What that leaves refused is the case +// worth refusing — a recipient nobody asked for. +func recipientsMatch(sent composeSent, got addressedEnvelope) bool { + return sameAddresses(sent.To, got.To) && + sameAddresses(sent.CC, got.CC) && + addressSubset(got.BCC, sent.BCC) +} + +func sameAddresses(want, got []string) bool { + if len(want) != len(got) { + return false + } + return addressSubset(got, want) +} + +func addressSubset(got, want []string) bool { + allowed := make(map[string]struct{}, len(want)) + for _, address := range want { + allowed[normalizeAddress(address)] = struct{}{} + } + for _, address := range got { + if _, ok := allowed[normalizeAddress(address)]; !ok { + return false + } + } + return true +} + +// normalizeAddress folds case, which is how every mail host in practice treats an +// address, so a message that came back as Alice@example.com is not reported as having +// reached somebody else. +func normalizeAddress(address string) string { + return strings.ToLower(strings.TrimSpace(address)) +} + +// composeResult is the machine contract a send answers with: the handle, and what +// reading the message back showed. `sent` is always true here — a send that was not +// accepted is an error, and one that was accepted but named nothing is an ambiguous +// error, never this. +type composeResult struct { + Sent bool `json:"sent"` + MessageID int64 `json:"message_id,omitempty"` + TopicID int64 `json:"topic_id,omitempty"` + AppURL string `json:"app_url,omitempty"` + Verification composeVerification `json:"verification"` +} + +// composeResultFor assembles the answer from the handle the send named and the message +// that was read back. The readback fills in a thread id and an app URL the send's own +// response did not carry — a message names its thread in its URL — and never overrides +// one it did. +func composeResultFor(handle composeHandle, verification composeVerification, message *generated.Message) composeResult { + result := composeResult{ + Sent: true, + MessageID: handle.MessageID, + TopicID: handle.TopicID, + AppURL: handle.AppURL, + Verification: verification, + } + if message != nil { + if result.AppURL == "" { + result.AppURL = message.Url + } + if result.TopicID == 0 { + result.TopicID = mail.TopicIDIn(message.Url) + } + } + return result +} diff --git a/internal/cmd/compose_verify_test.go b/internal/cmd/compose_verify_test.go new file mode 100644 index 00000000..f5ef8940 --- /dev/null +++ b/internal/cmd/compose_verify_test.go @@ -0,0 +1,495 @@ +package cmd + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/basecamp/hey-cli/internal/apierr" + "github.com/basecamp/hey-cli/internal/htmlutil" +) + +// customerUpdateMarkdown is the portable subset an automated sender writes in: +// paragraphs, `- ` bullets, **bold** and *italic*, and nothing else. +const customerUpdateMarkdown = `Hi Alice, + +Here is this week's update from the Nova desk. + +- **Shipped** the billing import +- *Started* the audit log +- Fixed twelve reconciliation bugs + +Next week we move on to the export pipeline.` + +// composeSend is what a test server saw, and what it was told to answer with. +type composeSend struct { + Path string + Subject string + Content string + ActingSenderID int64 + To []string + CC []string + BCC []string + + // SendStatus, SendLocation and SendBody are what POST /messages.json answers. + // The zero value is HEY's own draft-save shape: 204 with a Location naming the + // entry. + SendStatus int + SendLocation string + SendBody string + + // ReadbackStatus and ReadbackJSON are what GET /messages/{id}.json answers. + // An empty ReadbackJSON echoes what was posted back as the stored message. + ReadbackStatus int + ReadbackJSON string + + // Reads counts the readbacks, so a test can say the message was fetched once. + Reads int +} + +// composeSendServer answers the identity a send needs, the send itself, and the +// readback of the message it created. +func composeSendServer(t *testing.T) (*httptest.Server, *composeSend) { + t.Helper() + sent := &composeSend{SendStatus: http.StatusNoContent, SendLocation: "https://app.hey.com/messages/9101"} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/messages"): + var body struct { + ActingSenderID int64 `json:"acting_sender_id"` + Message struct { + Subject string `json:"subject"` + Content string `json:"content"` + } `json:"message"` + Entry struct { + Status string `json:"status"` + Addressed struct { + Directly []string `json:"directly"` + Copied []string `json:"copied"` + Blindcopied []string `json:"blindcopied"` + } `json:"addressed"` + } `json:"entry"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + sent.Path = r.URL.Path + sent.Subject = body.Message.Subject + sent.Content = body.Message.Content + sent.ActingSenderID = body.ActingSenderID + sent.To = body.Entry.Addressed.Directly + sent.CC = body.Entry.Addressed.Copied + sent.BCC = body.Entry.Addressed.Blindcopied + if sent.SendLocation != "" { + w.Header().Set("Location", sent.SendLocation) + } + if sent.SendBody != "" { + w.Header().Set("Content-Type", "application/json") + } + w.WriteHeader(sent.SendStatus) + if sent.SendBody != "" { + fmt.Fprint(w, sent.SendBody) + } + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/messages/"): + sent.Reads++ + if sent.ReadbackStatus != 0 && sent.ReadbackStatus != http.StatusOK { + w.Header().Set("Content-Type", "application/json") + http.Error(w, `{"message":"not found"}`, sent.ReadbackStatus) + return + } + w.Header().Set("Content-Type", "application/json") + if sent.ReadbackJSON != "" { + fmt.Fprint(w, sent.ReadbackJSON) + return + } + payload, _ := json.Marshal(map[string]any{ + "id": 9101, + "subject": sent.Subject, + "content": sent.Content, + "url": "https://app.hey.com/topics/7742", + "sender": map[string]any{ + "id": 42, "name": "Nova Desk", "email_address": "nova@example.com", + }, + "creator": map[string]any{ + "id": 42, "name": "Nova Desk", "email_address": "nova@example.com", + }, + "addressed": map[string]any{ + "directly": contactsFor(sent.To), + "copied": contactsFor(sent.CC), + }, + }) + _, _ = w.Write(payload) + case strings.Contains(r.URL.Path, "identity"): + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"id":1,"accounts":[{"id":8,"status":"active"}],"senders":[{"id":42,"account_id":8,"default":true}]}`) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.Error(w, "not found", http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + return server, sent +} + +func contactsFor(emails []string) []map[string]any { + contacts := make([]map[string]any, 0, len(emails)) + for i, email := range emails { + contacts = append(contacts, map[string]any{"id": 100 + i, "email_address": email}) + } + return contacts +} + +// composeEnvelope is the machine contract `hey compose --json` answers with. +type composeEnvelope struct { + OK bool `json:"ok"` + Data struct { + Sent bool `json:"sent"` + MessageID int64 `json:"message_id"` + TopicID int64 `json:"topic_id"` + AppURL string `json:"app_url"` + Verification struct { + Status string `json:"status"` + Method string `json:"method"` + Reason string `json:"reason"` + Subject string `json:"subject"` + Sender struct { + Name string `json:"name"` + EmailAddress string `json:"email_address"` + } `json:"sender"` + Recipients struct { + To []string `json:"to"` + CC []string `json:"cc"` + BCC []string `json:"bcc"` + BCCDisclosed bool `json:"bcc_disclosed"` + Truncated bool `json:"truncated"` + } `json:"recipients"` + BodyMarkdown string `json:"body_markdown"` + BodyMarkdownSHA256 string `json:"body_markdown_sha256"` + BodyTruncated bool `json:"body_truncated"` + MatchesSent struct { + Subject bool `json:"subject"` + Body bool `json:"body"` + Recipients bool `json:"recipients"` + } `json:"matches_sent"` + } `json:"verification"` + } `json:"data"` + Summary string `json:"summary"` +} + +func composeJSON(t *testing.T, stdout string) composeEnvelope { + t.Helper() + var envelope composeEnvelope + if err := json.Unmarshal([]byte(stdout), &envelope); err != nil { + t.Fatalf("compose did not answer JSON (%v): %s", err, stdout) + } + return envelope +} + +// The whole contract in one run: a send answers a handle, the handle is read back, and +// the readback names the exact message, its sender, its recipients, its subject and its +// body as canonical Markdown. +func TestComposeAnswersAHandleAndVerifiesTheMessageItCreated(t *testing.T) { + server, sent := composeSendServer(t) + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--cc", "bob@example.com", + "--subject", "Inovo Customer Update — Week 12", + "-m", customerUpdateMarkdown) + if err != nil { + t.Fatalf("compose: %v", err) + } + + envelope := composeJSON(t, stdout) + if !envelope.OK || !envelope.Data.Sent { + t.Fatalf("envelope = %+v, want an ok send", envelope) + } + if envelope.Data.MessageID != 9101 { + t.Errorf("message_id = %d, want 9101 from the Location header", envelope.Data.MessageID) + } + if envelope.Data.TopicID != 7742 { + t.Errorf("topic_id = %d, want 7742 from the message's own URL", envelope.Data.TopicID) + } + if envelope.Data.AppURL != "https://app.hey.com/topics/7742" { + t.Errorf("app_url = %q", envelope.Data.AppURL) + } + if sent.Reads != 1 { + t.Errorf("readbacks = %d, want exactly one", sent.Reads) + } + + verification := envelope.Data.Verification + if verification.Status != "verified" { + t.Fatalf("status = %q (%s), want verified", verification.Status, verification.Reason) + } + if verification.Method != "message_read" { + t.Errorf("method = %q", verification.Method) + } + if verification.Sender.EmailAddress != "nova@example.com" { + t.Errorf("sender = %q, want the address HEY sent it as", verification.Sender.EmailAddress) + } + if verification.Subject != "Inovo Customer Update — Week 12" { + t.Errorf("subject = %q", verification.Subject) + } + if want := []string{"alice@example.com"}; !equalStrings(verification.Recipients.To, want) { + t.Errorf("to = %v, want %v", verification.Recipients.To, want) + } + if want := []string{"bob@example.com"}; !equalStrings(verification.Recipients.CC, want) { + t.Errorf("cc = %v, want %v", verification.Recipients.CC, want) + } + if len(verification.Recipients.BCC) != 0 || verification.Recipients.BCCDisclosed { + t.Errorf("bcc = %v disclosed = %v, want an undisclosed empty list", + verification.Recipients.BCC, verification.Recipients.BCCDisclosed) + } + if !verification.MatchesSent.Subject || !verification.MatchesSent.Body || !verification.MatchesSent.Recipients { + t.Errorf("matches_sent = %+v, want every comparison to hold", verification.MatchesSent) + } +} + +// The body comes back as the Markdown it was written in: paragraphs, `- ` bullets, +// **bold** and *italic* survive the round trip through HEY's Trix HTML byte for byte, +// which is what lets a caller compare what it sent with what was stored. +func TestComposeRoundTripsThePortableMarkdownSubset(t *testing.T) { + server, _ := composeSendServer(t) + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", + "-m", customerUpdateMarkdown) + if err != nil { + t.Fatalf("compose: %v", err) + } + + body := composeJSON(t, stdout).Data.Verification.BodyMarkdown + if body != customerUpdateMarkdown { + t.Errorf("body_markdown =\n%q\nwant\n%q", body, customerUpdateMarkdown) + } + for _, want := range []string{ + "Hi Alice,\n\nHere is this week's update", + "- **Shipped** the billing import", + "- *Started* the audit log", + } { + if !strings.Contains(body, want) { + t.Errorf("body_markdown does not carry %q", want) + } + } + if digest := composeJSON(t, stdout).Data.Verification.BodyMarkdownSHA256; digest != bodyDigest(customerUpdateMarkdown) { + t.Errorf("body_markdown_sha256 = %q, want the digest of the canonical Markdown", digest) + } +} + +// A send the server accepted but named nothing for is neither a success nor a failure: +// the message may exist. It is reported as ambiguous, with the code and exit status +// that say "do not retry, reconcile". +func TestComposeRefusesAnAcceptedSendItCannotReadBack(t *testing.T) { + server, sent := composeSendServer(t) + sent.SendLocation = "" + + _, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + + var cliErr *apierr.Error + if !errors.As(err, &cliErr) { + t.Fatalf("error = %v, want the CLI's typed error", err) + } + if cliErr.Code != apierr.CodeAmbiguous { + t.Errorf("code = %q, want %q", cliErr.Code, apierr.CodeAmbiguous) + } + if !strings.Contains(cliErr.Message, "may have been sent") { + t.Errorf("message = %q, want it to say the send may have landed", cliErr.Message) + } + if sent.Reads != 0 { + t.Errorf("readbacks = %d, want none — there is nothing to read", sent.Reads) + } +} + +// A readback that fails leaves the send reported with its handle and an honest +// unverified status: the message exists, we simply could not show it yet. +func TestComposeReportsAnUnreadableMessageAsUnverified(t *testing.T) { + server, sent := composeSendServer(t) + sent.ReadbackStatus = http.StatusNotFound + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + envelope := composeJSON(t, stdout) + if envelope.Data.MessageID != 9101 { + t.Errorf("message_id = %d, want the handle to survive a failed readback", envelope.Data.MessageID) + } + if envelope.Data.Verification.Status != "unverified" { + t.Errorf("status = %q, want unverified", envelope.Data.Verification.Status) + } + if envelope.Data.Verification.Reason == "" { + t.Error("an unverified send must say why") + } + if envelope.Data.Verification.Subject != "" || len(envelope.Data.Verification.Recipients.To) != 0 { + t.Error("nothing was read back, so nothing may be reported as read back") + } +} + +// A recipient nobody asked for is the failure this contract exists to catch. +func TestComposeReportsAnUnexpectedRecipientAsAMismatch(t *testing.T) { + server, sent := composeSendServer(t) + sent.ReadbackJSON = `{ + "id": 9101, + "subject": "Inovo Customer Update — Week 12", + "content": "

Body.

", + "sender": {"id": 42, "name": "Nova Desk", "email_address": "nova@example.com"}, + "addressed": {"directly": [ + {"id": 100, "email_address": "alice@example.com"}, + {"id": 101, "email_address": "mallory@example.com"} + ]} + }` + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + verification := composeJSON(t, stdout).Data.Verification + if verification.Status != "mismatch" { + t.Errorf("status = %q, want mismatch", verification.Status) + } + if verification.MatchesSent.Recipients { + t.Error("recipients must not compare equal when the readback carries one nobody asked for") + } + if want := []string{"alice@example.com", "mallory@example.com"}; !equalStrings(verification.Recipients.To, want) { + t.Errorf("to = %v, want the readback's own list %v", verification.Recipients.To, want) + } +} + +// HEY does not serve a sent message's BCC line back. That is reported as undisclosed +// rather than as an empty list that was proved empty, and it does not fail the +// comparison: a caller may allow an omitted BCC while still refusing a recipient it +// did not ask for. +func TestComposeReportsAnUndisclosedBCCWithoutFabricatingIt(t *testing.T) { + server, _ := composeSendServer(t) + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + verification := composeJSON(t, stdout).Data.Verification + if verification.Recipients.BCCDisclosed { + t.Error("bcc_disclosed must be false when HEY served no blindcopied list") + } + if len(verification.Recipients.BCC) != 0 { + t.Errorf("bcc = %v, want nothing invented", verification.Recipients.BCC) + } + if !verification.MatchesSent.Recipients { + t.Error("an undisclosed BCC is not a mismatch") + } + if verification.Status != "verified" { + t.Errorf("status = %q, want verified", verification.Status) + } +} + +// A body that came back changed is a mismatch, not a verified send. +func TestComposeReportsAChangedBodyAsAMismatch(t *testing.T) { + server, sent := composeSendServer(t) + sent.ReadbackJSON = `{ + "id": 9101, + "subject": "Inovo Customer Update — Week 12", + "content": "

Something else entirely.

", + "sender": {"id": 42, "email_address": "nova@example.com"}, + "addressed": {"directly": [{"id": 100, "email_address": "alice@example.com"}]} + }` + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + verification := composeJSON(t, stdout).Data.Verification + if verification.Status != "mismatch" || verification.MatchesSent.Body { + t.Errorf("verification = %+v, want a body mismatch", verification) + } + if verification.BodyMarkdown != "Something else entirely." { + t.Errorf("body_markdown = %q, want what HEY actually stored", verification.BodyMarkdown) + } +} + +// A send HEY refused outright never reaches the handle logic: it keeps the terminal +// code it came with, so a caller can tell "rejected before delivery" from "accepted but +// unreadable". +func TestComposeKeepsATerminalRejectionSeparateFromAnAmbiguousOne(t *testing.T) { + server, sent := composeSendServer(t) + sent.SendStatus = http.StatusTooManyRequests + sent.SendLocation = "" + + _, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + + var cliErr *apierr.Error + if !errors.As(err, &cliErr) || cliErr.Code != apierr.CodeRateLimit { + t.Fatalf("error = %v, want a rate_limit error", err) + } +} + +// The line somebody reading along gets is the one they always got. +func TestComposeStyledOutputIsUnchanged(t *testing.T) { + server, _ := composeSendServer(t) + + stdout, _, err := runCLIRaw(t, server, "--styled", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + if strings.TrimSpace(stdout) != "Message sent." { + t.Errorf("styled output = %q, want the one line it has always written", stdout) + } +} + +// bodyDigest is over the canonical Markdown, so a test can state the expected digest +// without repeating the implementation. +func TestBodyDigestIsOverTheCanonicalMarkdown(t *testing.T) { + md := htmlutil.ToMarkdown(htmlutil.FromMarkdown(customerUpdateMarkdown)) + if bodyDigest(md.String()) != bodyDigest(customerUpdateMarkdown) { + t.Error("the portable subset must survive the round trip unchanged") + } +} + +func equalStrings(got, want []string) bool { + if len(got) != len(want) { + return false + } + for i := range got { + if got[i] != want[i] { + return false + } + } + return true +} + +// A body past the inline bound is left out rather than half-published: half a body +// invites a comparison that looks like it succeeded. The digest still covers the whole +// of it, so the body can be compared even when it cannot be shown. +func TestVerifiedBodyLeavesAnOversizedBodyToItsDigest(t *testing.T) { + small := "

Short enough.

" + body, truncated, digest := verifiedBody(small) + if truncated || body.String() != "Short enough." { + t.Errorf("body = %q truncated = %v, want the whole of a small body", body.String(), truncated) + } + if digest != bodyDigest("Short enough.") { + t.Errorf("digest = %q, want the digest of the canonical Markdown", digest) + } + + large := "

" + strings.Repeat("a", maxVerifiedBodyBytes+1) + "

" + body, truncated, digest = verifiedBody(large) + if !truncated { + t.Error("a body past the bound must say so") + } + if !body.IsEmpty() { + t.Error("a body past the bound must not be published in part") + } + if digest != bodyDigest(strings.Repeat("a", maxVerifiedBodyBytes+1)) { + t.Error("the digest must cover the whole body, bound or no bound") + } +} diff --git a/internal/cmd/thread_envelope_test.go b/internal/cmd/thread_envelope_test.go new file mode 100644 index 00000000..a70a5098 --- /dev/null +++ b/internal/cmd/thread_envelope_test.go @@ -0,0 +1,190 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// threadEnvelopeServer answers thread 7 as one entry whose message carries the whole +// addressing envelope: who sent it, who it went to, who was copied, and its subject. +func threadEnvelopeServer(t *testing.T, messageJSON string) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.URL.Path == "/topics/7/entries.json": + fmt.Fprint(w, `[{"id":9101,"kind":"message","summary":"Here is this week's update","created_at":"2026-04-12T09:30:00Z"}]`) + case strings.HasPrefix(r.URL.Path, "/messages/"): + fmt.Fprint(w, messageJSON) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.Error(w, "not found", http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + return server +} + +const customerUpdateMessageJSON = `{ + "id": 9101, + "subject": "Inovo Customer Update — Week 12", + "content": "

Hi Alice,

\n
    \n
  • Shipped the billing import
    • \n
", + "creator": {"id": 42, "name": "Nova Desk", "email_address": "nova@example.com"}, + "sender": {"id": 42, "name": "Nova Desk", "email_address": "nova@example.com"}, + "addressed": { + "directly": [{"id": 100, "name": "Alice Nakamura", "email_address": "alice@example.com"}], + "copied": [{"id": 101, "name": "Bob Okonkwo", "email_address": "bob@example.com"}] + } +}` + +// threadReadEnvelope is what `hey thread read --json` answers per entry, for a caller +// that has to prove which message it is looking at and who it reached. +type threadReadEnvelope struct { + Data []struct { + ID int64 `json:"id"` + Subject string `json:"subject"` + Sender struct { + Name string `json:"name"` + EmailAddress string `json:"email_address"` + } `json:"sender"` + Creator struct { + EmailAddress string `json:"email_address"` + } `json:"creator"` + Addressed struct { + To []string `json:"to"` + CC []string `json:"cc"` + BCC []string `json:"bcc"` + BCCDisclosed bool `json:"bcc_disclosed"` + Truncated bool `json:"truncated"` + } `json:"addressed"` + Body string `json:"body"` + BodyState string `json:"body_state"` + } `json:"data"` +} + +// The recipients are read off the message HEY served, not inferred from a position in a +// list or found by searching the body. +func TestThreadReadNamesTheEntrysSenderSubjectAndRecipients(t *testing.T) { + server := threadEnvelopeServer(t, customerUpdateMessageJSON) + + stdout, _, err := runCLIRaw(t, server, "--json", "thread", "read", "7") + if err != nil { + t.Fatalf("thread read: %v", err) + } + + var envelope threadReadEnvelope + if err := json.Unmarshal([]byte(stdout), &envelope); err != nil { + t.Fatalf("thread read did not answer JSON (%v): %s", err, stdout) + } + if len(envelope.Data) != 1 { + t.Fatalf("entries = %d, want 1", len(envelope.Data)) + } + + entry := envelope.Data[0] + if entry.ID != 9101 { + t.Errorf("id = %d, want 9101", entry.ID) + } + if entry.Subject != "Inovo Customer Update — Week 12" { + t.Errorf("subject = %q", entry.Subject) + } + if entry.Sender.EmailAddress != "nova@example.com" { + t.Errorf("sender = %q, want the address the message went out as", entry.Sender.EmailAddress) + } + if want := []string{"alice@example.com"}; !equalStrings(entry.Addressed.To, want) { + t.Errorf("to = %v, want %v", entry.Addressed.To, want) + } + if want := []string{"bob@example.com"}; !equalStrings(entry.Addressed.CC, want) { + t.Errorf("cc = %v, want %v", entry.Addressed.CC, want) + } + if len(entry.Addressed.BCC) != 0 || entry.Addressed.BCCDisclosed { + t.Errorf("bcc = %v disclosed = %v, want an undisclosed empty list", + entry.Addressed.BCC, entry.Addressed.BCCDisclosed) + } + if !strings.Contains(entry.Body, "- **Shipped** the billing import") { + t.Errorf("body = %q, want the Markdown the HTML converts to", entry.Body) + } + if entry.BodyState != "hydrated" { + t.Errorf("body_state = %q", entry.BodyState) + } +} + +// A BCC HEY does serve back is reported as disclosed, so a caller can tell the two +// cases apart rather than reading an empty list as proof. +func TestThreadReadSaysWhenABCCLineWasServed(t *testing.T) { + server := threadEnvelopeServer(t, `{ + "id": 9101, + "subject": "Inovo Customer Update — Week 12", + "content": "

Hi Alice,

", + "sender": {"id": 42, "email_address": "nova@example.com"}, + "addressed": { + "directly": [{"id": 100, "email_address": "alice@example.com"}], + "blindcopied": [{"id": 102, "email_address": "carol@example.org"}] + } + }`) + + stdout, _, err := runCLIRaw(t, server, "--json", "thread", "read", "7") + if err != nil { + t.Fatalf("thread read: %v", err) + } + var envelope threadReadEnvelope + if err := json.Unmarshal([]byte(stdout), &envelope); err != nil { + t.Fatalf("thread read did not answer JSON (%v): %s", err, stdout) + } + entry := envelope.Data[0] + if !entry.Addressed.BCCDisclosed { + t.Error("bcc_disclosed must be true when HEY served a blindcopied list") + } + if want := []string{"carol@example.org"}; !equalStrings(entry.Addressed.BCC, want) { + t.Errorf("bcc = %v, want %v", entry.Addressed.BCC, want) + } +} + +// A count reads the index alone, so there is no message to take an envelope from and +// none is invented. +func TestThreadReadCountCarriesNoEnvelope(t *testing.T) { + server := threadEnvelopeServer(t, customerUpdateMessageJSON) + + stdout, _, err := runCLIRaw(t, server, "--count", "thread", "read", "7") + if err != nil { + t.Fatalf("thread read --count: %v", err) + } + if strings.TrimSpace(stdout) != "1" { + t.Errorf("count = %q, want 1", stdout) + } +} + +// A recipient list longer than the bound is cut to it and says so, rather than a +// message addressed to a mailing list being retained whole. +func TestThreadReadBoundsTheRecipientsItRetains(t *testing.T) { + recipients := make([]string, 0, maxRetainedRecipients+5) + for i := range maxRetainedRecipients + 5 { + recipients = append(recipients, fmt.Sprintf(`{"id":%d,"email_address":"reader%d@example.com"}`, 200+i, i)) + } + server := threadEnvelopeServer(t, fmt.Sprintf(`{ + "id": 9101, + "subject": "Inovo Customer Update — Week 12", + "content": "

Hi.

", + "sender": {"id": 42, "email_address": "nova@example.com"}, + "addressed": {"directly": [%s]} + }`, strings.Join(recipients, ","))) + + stdout, _, err := runCLIRaw(t, server, "--json", "thread", "read", "7") + if err != nil { + t.Fatalf("thread read: %v", err) + } + var envelope threadReadEnvelope + if err := json.Unmarshal([]byte(stdout), &envelope); err != nil { + t.Fatalf("thread read did not answer JSON (%v): %s", err, stdout) + } + entry := envelope.Data[0] + if len(entry.Addressed.To) != maxRetainedRecipients { + t.Errorf("to = %d addresses, want the bound of %d", len(entry.Addressed.To), maxRetainedRecipients) + } + if !entry.Addressed.Truncated { + t.Error("a cut list must say it was cut") + } +} diff --git a/internal/cmd/thread_reply_test.go b/internal/cmd/thread_reply_test.go index 0deccfe4..07ae3094 100644 --- a/internal/cmd/thread_reply_test.go +++ b/internal/cmd/thread_reply_test.go @@ -100,6 +100,9 @@ func threadReplyServer(t *testing.T, messageJSON string, entryIDs ...int64) (*ht w.WriteHeader(http.StatusNoContent) return } + // A delivered reply answers the way a saved one does: the entry it created + // is named in Location, and nowhere else. + w.Header().Set("Location", "https://app.hey.com/messages/9101") w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated) fmt.Fprint(w, `{}`) @@ -117,6 +120,15 @@ func threadReplyServer(t *testing.T, messageJSON string, entryIDs ...int64) (*ht entries = append(entries, fmt.Sprintf(`{"id":%d}`, id)) } fmt.Fprintf(w, `{"id":7,"account_id":9,"entries":[%s]}`, strings.Join(entries, ",")) + case r.URL.Path == "/messages/9101.json": + // The reply read back after it was sent, as compose verifies it. + w.Header().Set("Content-Type", "application/json") + payload, _ := json.Marshal(map[string]any{ + "id": 9101, "subject": sent.Subject, "content": sent.Content, + "sender": map[string]any{"id": 42, "email_address": "jane@example.com"}, + "addressed": map[string]any{"directly": jsonContacts(sent.To), "copied": jsonContacts(sent.CC)}, + }) + _, _ = w.Write(payload) case strings.HasPrefix(r.URL.Path, "/messages/"): sent.MessageAccountFilter = r.URL.Query().Get("filtered_account_id") if r.URL.Path != "/messages/12.json" { @@ -133,6 +145,15 @@ func threadReplyServer(t *testing.T, messageJSON string, entryIDs ...int64) (*ht return server, sent } +// jsonContacts is a recipient list in the shape HEY serves one. +func jsonContacts(emails []string) []map[string]any { + contacts := make([]map[string]any, 0, len(emails)) + for i, email := range emails { + contacts = append(contacts, map[string]any{"id": 200 + i, "email_address": email}) + } + return contacts +} + // withSDKPointedAt builds the package-level client the commands use, aimed at a test // server, and puts the old one back afterwards. func withSDKPointedAt(t *testing.T, server *httptest.Server) { diff --git a/internal/cmd/topic.go b/internal/cmd/topic.go index b315f128..8bbc17c2 100644 --- a/internal/cmd/topic.go +++ b/internal/cmd/topic.go @@ -30,18 +30,29 @@ type threadContact struct { // HEY's Trix HTML; BodyHTML keeps that HTML for --html. BodyState says what became of // the body: hydrated, bodyless (HEY served none), over_limit or failed (it was not // read), or not_requested (the format did not need it). +// +// Subject, Sender and Addressed are the message's own addressing envelope, and they +// come from the message HEY served for the entry rather than from anything inferred: +// the entry index carries neither. They are therefore present exactly when the body +// was read — a count or a list of IDs reads no messages and invents no envelope — and +// they are what lets a caller prove which message it is looking at and who it reached. +// Creator is still whoever wrote the entry; Sender is the identity it went out as, +// which on a shared or alternate address is not the same person. type threadEntry struct { - ID int64 `json:"id"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` - Creator threadContact `json:"creator"` - AlternativeSenderName string `json:"alternative_sender_name"` - Summary string `json:"summary"` - Kind string `json:"kind"` - AppURL string `json:"app_url"` - Body htmlutil.Markdown `json:"body,omitzero"` - BodyState string `json:"body_state,omitempty"` - BodyHTML string `json:"-"` + ID int64 `json:"id"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` + Creator threadContact `json:"creator"` + Sender *threadContact `json:"sender,omitempty"` + AlternativeSenderName string `json:"alternative_sender_name"` + Subject string `json:"subject,omitempty"` + Addressed *addressedEnvelope `json:"addressed,omitempty"` + Summary string `json:"summary"` + Kind string `json:"kind"` + AppURL string `json:"app_url"` + Body htmlutil.Markdown `json:"body,omitzero"` + BodyState string `json:"body_state,omitempty"` + BodyHTML string `json:"-"` } type topicCommand struct { @@ -332,8 +343,15 @@ func newThreadEntry(loaded *threadload.Entry, html bool) threadEntry { appURL := entry.AppUrl var body htmlutil.Markdown bodyHTML := "" + subject := "" + var sender *threadContact + var addressed *addressedEnvelope if message := loaded.Message; message != nil { + subject = message.Subject + sender = senderOf(message) + envelope := addressedFrom(message.Addressed) + addressed = &envelope if creator.Id == 0 { creator = message.Creator } @@ -363,6 +381,9 @@ func newThreadEntry(loaded *threadload.Entry, html bool) threadEntry { CreatedAt: formatTimestamp(createdAt), UpdatedAt: formatTimestamp(updatedAt), AlternativeSenderName: entry.AlternativeSenderName, + Subject: subject, + Sender: sender, + Addressed: addressed, Summary: summary, Kind: entry.Kind, AppURL: appURL, diff --git a/internal/threadload/threadload.go b/internal/threadload/threadload.go index aea4d851..b1488e2c 100644 --- a/internal/threadload/threadload.go +++ b/internal/threadload/threadload.go @@ -401,10 +401,16 @@ func boundedError(err error) error { return errors.New(message) } -// retained is the part of a message a thread keeps — the body and what names the -// entry — and how many bytes it costs the budget. The rest of what HEY serves on a -// message, the recipient lists above all, is dropped here rather than held: the budget -// bounds what is kept, so it has to be charged for everything that is. +// retained is the part of a message a thread keeps — the body, what names the entry, +// and the addressing envelope: who it went out as and who it reached. The rest of what +// HEY serves on a message is dropped here rather than held. +// +// The envelope is kept because it is the only place a thread says who a message +// actually reached — the entry index carries none of it — and a caller that has to +// prove that cannot get it anywhere else. It is not kept for free: every address and +// name is charged to the byte budget, which is what bounds a thread addressed to a +// mailing list rather than a per-message cap that would silently cut a recipient list +// with nothing saying it had been cut. func retained(message *generated.Message) (*generated.Message, int64) { kept := &generated.Message{ Id: message.Id, @@ -412,18 +418,38 @@ func retained(message *generated.Message) (*generated.Message, int64) { Subject: message.Subject, Url: message.Url, Creator: message.Creator, + Sender: message.Sender, + Addressed: message.Addressed, CreatedAt: message.CreatedAt, UpdatedAt: message.UpdatedAt, } size := int64(len(kept.Content)+len(kept.Subject)+len(kept.Url)+ - len(kept.Creator.Name)+len(kept.Creator.EmailAddress)) + retainedOverhead + len(kept.Creator.Name)+len(kept.Creator.EmailAddress)+ + len(kept.Sender.Name)+len(kept.Sender.EmailAddress)) + + contactsSize(kept.Addressed.Directly) + + contactsSize(kept.Addressed.Copied) + + contactsSize(kept.Addressed.Blindcopied) + + retainedOverhead return kept, size } +// contactsSize is what a recipient list costs the budget: its addresses and names, and +// perContactOverhead for the struct holding them. +func contactsSize(contacts []generated.Contact) int64 { + var size int64 + for _, contact := range contacts { + size += int64(len(contact.Name)+len(contact.EmailAddress)) + perContactOverhead + } + return size +} + // retainedOverhead is what a kept message costs beyond its strings: the struct, the // entry, the timestamps. const retainedOverhead = 512 +// perContactOverhead is what one recipient costs beyond its name and address. +const perContactOverhead = 128 + type byteBudget struct { mu sync.Mutex remaining int64 diff --git a/internal/threadload/threadload_test.go b/internal/threadload/threadload_test.go index 6e29925d..81b8f31a 100644 --- a/internal/threadload/threadload_test.go +++ b/internal/threadload/threadload_test.go @@ -235,7 +235,7 @@ func TestLoadStopsAskingAfterTheBudgetRefuses(t *testing.T) { source := &fakeSource{pages: [][]int64{pages}, bodies: bodies} l := limits() l.Concurrency = 1 - l.MaxRetainedBytes = int64(len(pages))*(retainedOverhead+int64(len("summary 40")+len("message"))) + 3*(retainedOverhead+100) + 1 + l.MaxRetainedBytes = int64(len(pages))*(retainedOverhead+int64(len("summary 40")+len("message"))) + 3*(retainedOverhead+100+fakeRecipientCost) + 1 thread, err := Load(context.Background(), source, Request{TopicID: 7, Hydrate: true, Limits: l}) if err != nil { t.Fatal(err) @@ -279,7 +279,8 @@ func TestLoadBoundsTheReasonAFailedReadKeeps(t *testing.T) { } // What a thread keeps of a message is charged to the budget whole — body, subject, -// URL, creator — and what is not kept is not held: the recipient lists go. +// URL, creator, and the addressing envelope, which is kept because it is the only place +// a thread says who a message reached. func TestLoadChargesWhatItKeeps(t *testing.T) { source := &fakeSource{pages: [][]int64{{12, 11}}, bodies: map[int64]string{12: "x", 11: "y"}, subjects: map[int64]string{12: strings.Repeat("s", 200)}} l := limits() @@ -299,12 +300,16 @@ func TestLoadChargesWhatItKeeps(t *testing.T) { t.Fatal(err) } for _, entry := range thread.Entries { - if entry.Message == nil || len(entry.Message.Addressed.Directly) != 0 { - t.Errorf("entry %d kept %+v, want the body without the recipient lists", entry.Entry.Id, entry.Message) + if entry.Message == nil || len(entry.Message.Addressed.Directly) != 1 { + t.Errorf("entry %d kept %+v, want the body and its recipient list", entry.Entry.Id, entry.Message) } } } +// fakeRecipientCost is what the one recipient every fakeSource message carries costs +// the budget, so the arithmetic above states the cost rather than hiding it. +const fakeRecipientCost = perContactOverhead + int64(len("Rick Sanchez")) + func TestLoadStopsRequestingMessagesAtTheRequestCap(t *testing.T) { source := &fakeSource{pages: [][]int64{{14, 13, 12, 11}}, bodies: map[int64]string{14: "a", 13: "b", 12: "c", 11: "d"}} l := limits() @@ -340,7 +345,7 @@ func TestLoadSpendsTheByteBudgetNewestFirst(t *testing.T) { l := limits() // Four index entries and the two newest bodies fit; the third body does not. indexEntry := retainedOverhead + int64(len("summary 14")+len("message")) - l.MaxRetainedBytes = 4*indexEntry + 2*(retainedOverhead+10) + 1 + l.MaxRetainedBytes = 4*indexEntry + 2*(retainedOverhead+10+fakeRecipientCost) + 1 for range 20 { thread, err := Load(context.Background(), source, Request{TopicID: 7, Hydrate: true, Limits: l}) if err != nil { diff --git a/tests/smoke/compose_test.go b/tests/smoke/compose_test.go index 8e9229db..ea482f15 100644 --- a/tests/smoke/compose_test.go +++ b/tests/smoke/compose_test.go @@ -6,14 +6,47 @@ import ( "testing" ) +// composeResult is the machine contract `hey compose --json` answers with: the handle +// naming the message that was created, and what reading it back showed. +type composeResult struct { + Sent bool `json:"sent"` + MessageID int64 `json:"message_id"` + TopicID int64 `json:"topic_id"` + AppURL string `json:"app_url"` + Verification struct { + Status string `json:"status"` + Method string `json:"method"` + Reason string `json:"reason"` + Subject string `json:"subject"` + Sender struct { + Name string `json:"name"` + EmailAddress string `json:"email_address"` + } `json:"sender"` + Recipients struct { + To []string `json:"to"` + CC []string `json:"cc"` + BCC []string `json:"bcc"` + BCCDisclosed bool `json:"bcc_disclosed"` + } `json:"recipients"` + BodyMarkdown string `json:"body_markdown"` + BodyMarkdownSHA256 string `json:"body_markdown_sha256"` + MatchesSent struct { + Subject bool `json:"subject"` + Body bool `json:"body"` + Recipients bool `json:"recipients"` + } `json:"matches_sent"` + } `json:"verification"` +} + func TestCompose(t *testing.T) { uid := uniqueID() subject := fmt.Sprintf("Smoke test %s", uid) + body := "Hello from smoke test.\n\n- **First** item\n- *Second* item" stdout, stderr, code := hey(t, "compose", "--to", "david@basecamp.com", "--subject", subject, - "-m", "Hello from smoke test", + "-m", body, "--json", ) if code != 0 { @@ -29,14 +62,74 @@ func TestCompose(t *testing.T) { } assertContains(t, resp.Summary, "Message sent") - // Cross-verify: fetch the thread page and check the subject appears. - composeData := dataAs[map[string]any](t, resp) - if appURL, ok := composeData["app_url"].(string); ok { - topicID := extractTopicID(appURL) - if topicID != "" { - html := fetchHTML(t, fmt.Sprintf("%s/topics/%s", baseURL, topicID)) - assertContains(t, html, subject) + // The handle is not optional. A send that reports success and names nothing is the + // regression this check exists for: the cross-verification below used to sit inside + // an `if app_url, ok := …`, so a compose with no handle passed silently. + result := dataAs[composeResult](t, resp) + if !result.Sent { + t.Fatal("compose did not report the message as sent") + } + if result.MessageID == 0 && result.TopicID == 0 { + t.Fatalf("compose named neither a message nor a thread: %s", string(resp.Data)) + } + + // And the readback is what proves it. Anything but `verified` means the CLI could + // not show that what HEY stored is what was asked for. + verification := result.Verification + if verification.Status != "verified" { + t.Fatalf("verification = %s (%s), want verified", verification.Status, verification.Reason) + } + if verification.Subject != subject { + t.Errorf("verified subject = %q, want %q", verification.Subject, subject) + } + if verification.Sender.EmailAddress == "" { + t.Error("a verified send must name the address it went out as") + } + if len(verification.Recipients.To) != 1 || verification.Recipients.To[0] != "david@basecamp.com" { + t.Errorf("verified recipients = %v, want exactly the address it was sent to", verification.Recipients.To) + } + if !verification.MatchesSent.Subject || !verification.MatchesSent.Body || !verification.MatchesSent.Recipients { + t.Errorf("matches_sent = %+v, want every comparison to hold", verification.MatchesSent) + } + // The Markdown a caller wrote is the Markdown it gets back. + assertContains(t, verification.BodyMarkdown, "- **First** item") + assertContains(t, verification.BodyMarkdown, "- *Second* item") + if verification.BodyMarkdownSHA256 == "" { + t.Error("a verified send must carry the digest of what was stored") + } + + // Cross-verify against the browser: the thread the send named holds the subject. + if result.TopicID == 0 { + t.Fatalf("compose named no thread to cross-verify: %s", string(resp.Data)) + } + html := fetchHTML(t, fmt.Sprintf("%s/topics/%d", baseURL, result.TopicID)) + assertContains(t, html, subject) + + // And `hey thread read` answers the same envelope for the same message. + threadResp := heyJSON(t, "thread", "read", fmt.Sprintf("%d", result.TopicID)) + type threadEntry struct { + ID int64 `json:"id"` + Subject string `json:"subject"` + Addressed struct { + To []string `json:"to"` + } `json:"addressed"` + } + entries := dataAs[[]threadEntry](t, threadResp) + found := false + for _, entry := range entries { + if entry.ID != result.MessageID { + continue + } + found = true + if entry.Subject != subject { + t.Errorf("thread read subject = %q, want %q", entry.Subject, subject) } + if len(entry.Addressed.To) != 1 || entry.Addressed.To[0] != "david@basecamp.com" { + t.Errorf("thread read recipients = %v, want the address it was sent to", entry.Addressed.To) + } + } + if !found && result.MessageID != 0 { + t.Errorf("thread %d does not carry the message %d that compose named", result.TopicID, result.MessageID) } } From d7bdee48756731065977e3fbecc645e7acefe237 Mon Sep 17 00:00:00 2001 From: jonata97 Date: Wed, 2 Sep 2026 15:11:51 -0300 Subject: [PATCH 2/5] fix(compose): report whether HEY served the BCC line, not whether anybody was on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- docs/cli.md | 4 +- internal/cmd/addressed.go | 27 ++++- internal/cmd/addressed_test.go | 170 +++++++++++++++++++++++++++ internal/cmd/compose_verify.go | 20 +++- internal/cmd/compose_verify_test.go | 144 +++++++++++++++++++++++ internal/cmd/thread_envelope_test.go | 101 ++++++++++++++++ tests/smoke/compose_test.go | 9 ++ 7 files changed, 461 insertions(+), 14 deletions(-) create mode 100644 internal/cmd/addressed_test.go diff --git a/docs/cli.md b/docs/cli.md index 086debbd..58c788d1 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -263,9 +263,9 @@ Writing is Markdown too, everywhere text goes in: `-m`, `--content`, `--note`, p Drafts are the review-before-send lane: `hey compose --draft` (and `hey reply --draft`) saves instead of sending — recipients optional on a draft — and answers the draft's ID. `hey draft show` reads it back with the body as Markdown, `hey draft edit` revises it (each flag replaces its field; what is not flagged is kept, by reading the draft and resending the whole of it, since a revision is not a patch on HEY's side), `hey draft send` delivers through HEY's undo window, and `hey draft delete` trashes it. Scheduling a delivery is done in a HEY app for now — the API cannot yet name an exact instant — and a schedule set there survives CLI edits untouched. A draft prepared here is reviewed and sent from any HEY app, which is the workflow this is for: an agent writes, a person decides. -A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` records whether HEY served the delivered message's BCC line. A send HEY accepted but named no message for is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. `--styled` output is unchanged: one line, `Message sent.` +A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` says whether HEY served the BCC field at all, not whether anybody was on it: an explicitly empty field is disclosed, while an omitted or null field is not. So `bcc: []` with `bcc_disclosed: true` proves nobody was blind-copied; the same list with `bcc_disclosed: false` proves nothing. A send HEY accepted but named no message for is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. `--styled` output is unchanged: one line, `Message sent.` -`hey thread read --json` carries the same envelope per entry, off the same message record: `subject`, `sender` (the identity the message went out as, next to `creator`, who wrote it) and `addressed` with `to`, `cc`, `bcc` and `bcc_disclosed`. Recipient lists are read from what HEY served, never inferred from a position or from the body, and are cut at a hundred addresses per line with `truncated` saying so. `--count` and `--ids-only` read no messages, so they carry no envelope rather than an invented one. +`hey thread read --json` carries the same envelope per entry, off the same message record: `subject`, `sender` (the identity the message went out as, next to `creator`, who wrote it) and `addressed` with `to`, `cc`, `bcc` and `bcc_disclosed`, with the same field-presence meaning as on a send. Recipient lists are read from what HEY served, never inferred from a position or from the body, and are cut at a hundred addresses per line with `truncated` saying so. `--count` and `--ids-only` read no messages, so they carry no envelope rather than an invented one. `hey share ` gets a sharing link for a thread. Anyone with the link can see the entire thread and future emails or replies sent to it. `hey unshare ` turns off the sharing link. diff --git a/internal/cmd/addressed.go b/internal/cmd/addressed.go index a7a6c4f7..c8e2f3b9 100644 --- a/internal/cmd/addressed.go +++ b/internal/cmd/addressed.go @@ -16,11 +16,16 @@ const maxRetainedRecipients = 100 // meant to and nowhere else, so it is read off the message HEY answered with rather // than inferred from a position in a list or found in the body. // -// BCCDisclosed is the honest part. HEY does not distinguish "there was no BCC" from "we -// are not telling you the BCC" — both arrive as no blindcopied recipients at all — so -// BCC being empty is never proof that nobody was blind-copied. BCCDisclosed is true only -// when HEY actually served addresses, and a caller that must not accept an unexpected -// recipient reads it before treating an empty BCC as settled. +// BCCDisclosed is the honest part, and it answers "did HEY tell us the BCC line" rather +// than "was anybody on it". Those are different questions, and reading the second as the +// first left a caller unable to prove a message's exact destinations: an empty BCC HEY +// served — which is proof that nobody was blind-copied — had the same shape as one HEY +// withheld, which proves nothing at all. +// +// So it is presence, not population: true when the blindcopied field arrived, an +// explicitly empty array included, and false when it was omitted, null, or there was no +// addressing at all. A caller that must not accept an unexpected recipient reads it +// before treating an empty BCC as settled — false still means unknown. type addressedEnvelope struct { To []string `json:"to"` CC []string `json:"cc"` @@ -31,6 +36,16 @@ type addressedEnvelope struct { } // addressedFrom describes HEY's addressing in the CLI's shape, within the bound. +// +// Disclosure is read from the decoded slice's nil-ness, which is where the presence of +// the field survives: encoding/json leaves an omitted or null array nil and makes an +// explicit `[]` non-nil, generated.Message declares no unmarshaler of its own to flatten +// the two, and threadload's retained copies the slice header rather than rebuilding it. +// It is deliberately not read from len(bcc) — that would report a line HEY served as +// empty as withheld — nor from what the caller asked to send, which is not evidence of +// anything the server did. TestBlindcopiedPresenceSurvivesTheSDKDecode pins the +// invariant through a real HTTP response, since it is the kind that would otherwise +// break in silence. func addressedFrom(addressed generated.Addressed) addressedEnvelope { to, toCut := boundedEmails(addressed.Directly) cc, ccCut := boundedEmails(addressed.Copied) @@ -39,7 +54,7 @@ func addressedFrom(addressed generated.Addressed) addressedEnvelope { To: to, CC: cc, BCC: bcc, - BCCDisclosed: len(bcc) > 0, + BCCDisclosed: addressed.Blindcopied != nil, Truncated: toCut || ccCut || bccCut, } } diff --git a/internal/cmd/addressed_test.go b/internal/cmd/addressed_test.go new file mode 100644 index 00000000..285b7de8 --- /dev/null +++ b/internal/cmd/addressed_test.go @@ -0,0 +1,170 @@ +package cmd + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/basecamp/hey-sdk/go/pkg/generated" +) + +// messageServingAddressed answers one message whose `addressed` object is exactly the +// JSON given, so a test can say what HEY put on the wire rather than what a Go struct +// looks like after somebody built it by hand. +func messageServingAddressed(t *testing.T, addressedJSON string) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasPrefix(r.URL.Path, "/messages/") { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.Error(w, "not found", http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"id":9101,"subject":"Inovo Customer Update — Week 12", + "content":"

Hi Alice,

", + "sender":{"id":42,"email_address":"nova@example.com"}%s}`, addressedJSON) + })) + t.Cleanup(server.Close) + return server +} + +// bcc_disclosed rests on one fact about the decode: `encoding/json` leaves a field HEY +// omitted — or served as null — nil, and makes an explicitly empty array non-nil, and +// nothing between HEY's JSON and generated.Message flattens the two together. That is +// asserted here through the real path — an HTTP response read by the SDK — because a +// struct built by hand would prove nothing about the decoder, and this is the invariant +// that would break silently if the SDK ever changed decoders. +func TestBlindcopiedPresenceSurvivesTheSDKDecode(t *testing.T) { + tests := []struct { + name string + addressed string + wantNonNil bool + wantContacts int + }{ + { + name: "the addressed object itself is omitted", + addressed: ``, + }, + { + name: "blindcopied is omitted", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}]}`, + }, + { + name: "blindcopied is null", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":null}`, + }, + { + name: "blindcopied is an explicitly empty array", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":[]}`, + wantNonNil: true, + }, + { + name: "blindcopied carries addresses", + addressed: `,"addressed":{"blindcopied":[{"id":102,"email_address":"carol@example.org"}]}`, + wantNonNil: true, + wantContacts: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := messageServingAddressed(t, tt.addressed) + withSDKPointedAt(t, server) + + message, err := sdk.Messages().Get(context.Background(), 9101) + if err != nil { + t.Fatalf("read the message: %v", err) + } + if got := message.Addressed.Blindcopied != nil; got != tt.wantNonNil { + t.Errorf("blindcopied non-nil = %v, want %v", got, tt.wantNonNil) + } + if len(message.Addressed.Blindcopied) != tt.wantContacts { + t.Errorf("blindcopied = %d contacts, want %d", + len(message.Addressed.Blindcopied), tt.wantContacts) + } + }) + } +} + +// bcc_disclosed answers "did HEY tell us the BCC line", not "was anybody on it". Those +// are different questions, and reading the second as the first is what left a caller +// unable to prove a message's exact destinations: an empty BCC that HEY served and an +// empty BCC that HEY withheld had the same shape. +func TestAddressedFromReportsWhetherHEYServedABCCLineAtAll(t *testing.T) { + tests := []struct { + name string + blindcopied []generated.Contact + wantDisclosed bool + wantBCC []string + }{ + { + name: "no blindcopied field at all", + blindcopied: nil, + wantDisclosed: false, + wantBCC: []string{}, + }, + { + name: "an explicitly empty blindcopied line", + blindcopied: []generated.Contact{}, + wantDisclosed: true, + wantBCC: []string{}, + }, + { + name: "a blindcopied line with addresses", + blindcopied: []generated.Contact{{Id: 102, EmailAddress: "carol@example.org"}}, + wantDisclosed: true, + wantBCC: []string{"carol@example.org"}, + }, + { + // A contact HEY named without an address is still HEY answering the + // question: the line was served, it just carries nothing this can print. + name: "a blindcopied line whose only contact has no address", + blindcopied: []generated.Contact{{Id: 102}}, + wantDisclosed: true, + wantBCC: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + envelope := addressedFrom(generated.Addressed{Blindcopied: tt.blindcopied}) + if envelope.BCCDisclosed != tt.wantDisclosed { + t.Errorf("bcc_disclosed = %v, want %v", envelope.BCCDisclosed, tt.wantDisclosed) + } + if !equalStrings(envelope.BCC, tt.wantBCC) { + t.Errorf("bcc = %v, want %v", envelope.BCC, tt.wantBCC) + } + if envelope.BCC == nil { + t.Error("bcc must marshal as [] rather than null") + } + }) + } +} + +// Disclosure and the bound are independent: a line long enough to be cut was plainly +// served, so it is disclosed and truncated at once. +func TestAddressedFromKeepsDisclosureWhenALineIsCut(t *testing.T) { + contacts := make([]generated.Contact, 0, maxRetainedRecipients+5) + for i := range maxRetainedRecipients + 5 { + contacts = append(contacts, generated.Contact{ + Id: int64(200 + i), EmailAddress: fmt.Sprintf("reader%d@example.com", i), + }) + } + + envelope := addressedFrom(generated.Addressed{Blindcopied: contacts}) + if !envelope.BCCDisclosed { + t.Error("a line HEY served in full is disclosed however much of it is kept") + } + if !envelope.Truncated { + t.Error("a cut list must say it was cut") + } + if len(envelope.BCC) != maxRetainedRecipients { + t.Errorf("bcc = %d addresses, want the bound of %d", len(envelope.BCC), maxRetainedRecipients) + } + if envelope.BCC[0] != "reader0@example.com" { + t.Errorf("bcc[0] = %q, want the first address verbatim", envelope.BCC[0]) + } +} diff --git a/internal/cmd/compose_verify.go b/internal/cmd/compose_verify.go index 2d429c08..6de267a5 100644 --- a/internal/cmd/compose_verify.go +++ b/internal/cmd/compose_verify.go @@ -77,8 +77,11 @@ type composeMatches struct { Subject bool `json:"subject"` Body bool `json:"body"` // Recipients holds when the To and CC lines are exactly the ones asked for and - // every BCC address served back was asked for too. An undisclosed BCC does not - // break it — HEY commonly serves none — but a recipient nobody asked for does. + // every BCC address served back was asked for too. It is the "nobody unexpected" + // question, so a BCC line HEY did not serve does not break it and neither does a + // disclosed line that is shorter than what was asked for; a recipient nobody asked + // for does. Whether a requested BCC must also appear is the caller's policy, and + // Recipients.BCCDisclosed is what tells it whether the answer is evidence. Recipients bool `json:"recipients"` } @@ -186,10 +189,15 @@ func bodyDigest(markdown string) string { // recipientsMatch reports whether the message reached exactly who it was meant to. // // To and CC must be the sets that were asked for, in any order. BCC is one-sided: HEY -// serves no blindcopied line back for a message it delivered, and an empty list is not -// proof that nobody was blind-copied, so a BCC that came back must have been asked for -// but one that did not come back costs nothing. What that leaves refused is the case -// worth refusing — a recipient nobody asked for. +// may serve a delivered message's blindcopied line or withhold it, so a BCC that came +// back must have been asked for while one that did not come back costs nothing. What +// that leaves refused is the case worth refusing — a recipient nobody asked for. +// +// A caller that needs the stronger claim — that the BCC line is exactly what it asked +// for — reads addressedEnvelope.BCCDisclosed and the BCC list itself and decides for +// itself. This function deliberately does not make that call: an absent line and an +// empty one mean different things, and only the caller knows which it is willing to +// accept. func recipientsMatch(sent composeSent, got addressedEnvelope) bool { return sameAddresses(sent.To, got.To) && sameAddresses(sent.CC, got.CC) && diff --git a/internal/cmd/compose_verify_test.go b/internal/cmd/compose_verify_test.go index f5ef8940..06b31233 100644 --- a/internal/cmd/compose_verify_test.go +++ b/internal/cmd/compose_verify_test.go @@ -493,3 +493,147 @@ func TestVerifiedBodyLeavesAnOversizedBodyToItsDigest(t *testing.T) { t.Error("the digest must cover the whole body, bound or no bound") } } + +// The verification carries the same semantics as `hey thread read`: a BCC line HEY +// served is disclosed even when it is empty, and one HEY withheld is not. Without that, +// a caller cannot prove the message's exact destination set — an empty BCC it was told +// about and one it was not had the same shape. +func TestComposeVerificationDistinguishesAnEmptyBCCLineFromAWithheldOne(t *testing.T) { + tests := []struct { + name string + addressed string + wantDisclosed bool + wantBCC []string + }{ + { + name: "blindcopied is omitted", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}]}`, + }, + { + name: "blindcopied is null", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":null}`, + }, + { + name: "blindcopied is an explicitly empty array", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":[]}`, + wantDisclosed: true, + }, + { + name: "blindcopied carries the address it was sent to", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":[{"id":102,"email_address":"carol@example.org"}]}`, + wantDisclosed: true, + wantBCC: []string{"carol@example.org"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server, sent := composeSendServer(t) + sent.ReadbackJSON = fmt.Sprintf( + `{"id":9101,"subject":"Inovo Customer Update — Week 12","content":"

Body.

", + "url":"https://app.hey.com/topics/7742", + "sender":{"id":42,"name":"Nova Desk","email_address":"nova@example.com"}%s}`, tt.addressed) + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + verification := composeJSON(t, stdout).Data.Verification + if verification.Recipients.BCCDisclosed != tt.wantDisclosed { + t.Errorf("bcc_disclosed = %v, want %v", + verification.Recipients.BCCDisclosed, tt.wantDisclosed) + } + want := tt.wantBCC + if want == nil { + want = []string{} + } + if !equalStrings(verification.Recipients.BCC, want) { + t.Errorf("bcc = %v, want %v", verification.Recipients.BCC, want) + } + // Disclosure says nothing about the rest of the envelope, which is read the + // same way whatever the BCC line did. + if wantTo := []string{"alice@example.com"}; !equalStrings(verification.Recipients.To, wantTo) { + t.Errorf("to = %v, want %v", verification.Recipients.To, wantTo) + } + if verification.Subject != "Inovo Customer Update — Week 12" { + t.Errorf("subject = %q", verification.Subject) + } + if verification.Sender.EmailAddress != "nova@example.com" { + t.Errorf("sender = %q", verification.Sender.EmailAddress) + } + if verification.Status != "verified" { + t.Errorf("status = %q, want verified — every disclosure here is consistent with the send", verification.Status) + } + if !verification.MatchesSent.Recipients { + t.Error("no unexpected recipient came back, so the comparison holds") + } + }) + } +} + +// A readback carrying no addressing at all discloses nothing and proves nothing: every +// line is empty and undisclosed, and the To line the message was sent to did not come +// back, which is a mismatch rather than a quiet pass. +func TestComposeVerificationTreatsAMissingAddressedObjectAsUndisclosed(t *testing.T) { + server, sent := composeSendServer(t) + sent.ReadbackJSON = `{ + "id": 9101, "subject": "Inovo Customer Update — Week 12", "content": "

Body.

", + "sender": {"id": 42, "email_address": "nova@example.com"} + }` + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + verification := composeJSON(t, stdout).Data.Verification + if verification.Recipients.BCCDisclosed { + t.Error("nothing was served, so nothing is disclosed") + } + if len(verification.Recipients.To) != 0 || len(verification.Recipients.BCC) != 0 { + t.Errorf("recipients = %+v, want nothing invented", verification.Recipients) + } + if verification.MatchesSent.Recipients { + t.Error("the To line it was sent to did not come back, so the comparison must not hold") + } + if verification.Status != "mismatch" { + t.Errorf("status = %q, want mismatch", verification.Status) + } +} + +// A disclosed BCC line naming somebody nobody asked for is still the failure this +// contract exists to catch. +func TestComposeVerificationRefusesAnUnexpectedDisclosedBCC(t *testing.T) { + server, sent := composeSendServer(t) + sent.ReadbackJSON = `{ + "id": 9101, "subject": "Inovo Customer Update — Week 12", "content": "

Body.

", + "sender": {"id": 42, "email_address": "nova@example.com"}, + "addressed": { + "directly": [{"id": 100, "email_address": "alice@example.com"}], + "blindcopied": [{"id": 199, "email_address": "mallory@example.com"}] + } + }` + + stdout, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose: %v", err) + } + + verification := composeJSON(t, stdout).Data.Verification + if !verification.Recipients.BCCDisclosed { + t.Error("a served blindcopied line is disclosed even when it is wrong") + } + if verification.MatchesSent.Recipients { + t.Error("a blind-copied recipient nobody asked for must not compare equal") + } + if verification.Status != "mismatch" { + t.Errorf("status = %q, want mismatch", verification.Status) + } +} diff --git a/internal/cmd/thread_envelope_test.go b/internal/cmd/thread_envelope_test.go index a70a5098..82bf40b0 100644 --- a/internal/cmd/thread_envelope_test.go +++ b/internal/cmd/thread_envelope_test.go @@ -188,3 +188,104 @@ func TestThreadReadBoundsTheRecipientsItRetains(t *testing.T) { t.Error("a cut list must say it was cut") } } + +// The same question through the read a caller actually makes. `bcc_disclosed` says +// whether HEY served the BCC line, so an explicitly empty one — proof that nobody was +// blind-copied — is no longer the same shape as one HEY withheld. +func TestThreadReadDistinguishesAnEmptyBCCLineFromAWithheldOne(t *testing.T) { + tests := []struct { + name string + addressed string + wantDisclosed bool + wantBCC []string + }{ + { + name: "the addressed object is omitted entirely", + addressed: ``, + }, + { + name: "blindcopied is omitted", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}]}`, + }, + { + name: "blindcopied is null", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":null}`, + }, + { + name: "blindcopied is an explicitly empty array", + addressed: `,"addressed":{"directly":[{"id":100,"email_address":"alice@example.com"}],"blindcopied":[]}`, + wantDisclosed: true, + }, + { + name: "blindcopied carries addresses", + addressed: `,"addressed":{"blindcopied":[{"id":102,"email_address":"carol@example.org"}]}`, + wantDisclosed: true, + wantBCC: []string{"carol@example.org"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := threadEnvelopeServer(t, fmt.Sprintf( + `{"id":9101,"subject":"Inovo Customer Update — Week 12","content":"

Hi Alice,

", + "sender":{"id":42,"email_address":"nova@example.com"}%s}`, tt.addressed)) + + stdout, _, err := runCLIRaw(t, server, "--json", "thread", "read", "7") + if err != nil { + t.Fatalf("thread read: %v", err) + } + var envelope threadReadEnvelope + if err := json.Unmarshal([]byte(stdout), &envelope); err != nil { + t.Fatalf("thread read did not answer JSON (%v): %s", err, stdout) + } + addressed := envelope.Data[0].Addressed + if addressed.BCCDisclosed != tt.wantDisclosed { + t.Errorf("bcc_disclosed = %v, want %v", addressed.BCCDisclosed, tt.wantDisclosed) + } + want := tt.wantBCC + if want == nil { + want = []string{} + } + if !equalStrings(addressed.BCC, want) { + t.Errorf("bcc = %v, want %v", addressed.BCC, want) + } + // Whatever the BCC line said, the rest of the envelope is untouched. + if envelope.Data[0].Subject != "Inovo Customer Update — Week 12" { + t.Errorf("subject = %q", envelope.Data[0].Subject) + } + if envelope.Data[0].Sender.EmailAddress != "nova@example.com" { + t.Errorf("sender = %q", envelope.Data[0].Sender.EmailAddress) + } + }) + } +} + +// A BCC line long enough to be cut is disclosed and truncated at once: the bound is +// about what is kept, not about whether HEY answered. +func TestThreadReadKeepsBCCDisclosureWhenTheLineIsCut(t *testing.T) { + recipients := make([]string, 0, maxRetainedRecipients+5) + for i := range maxRetainedRecipients + 5 { + recipients = append(recipients, fmt.Sprintf(`{"id":%d,"email_address":"reader%d@example.org"}`, 300+i, i)) + } + server := threadEnvelopeServer(t, fmt.Sprintf(`{ + "id": 9101, "subject": "Inovo Customer Update — Week 12", "content": "

Hi.

", + "sender": {"id": 42, "email_address": "nova@example.com"}, + "addressed": {"blindcopied": [%s]} + }`, strings.Join(recipients, ","))) + + stdout, _, err := runCLIRaw(t, server, "--json", "thread", "read", "7") + if err != nil { + t.Fatalf("thread read: %v", err) + } + var envelope threadReadEnvelope + if err := json.Unmarshal([]byte(stdout), &envelope); err != nil { + t.Fatalf("thread read did not answer JSON (%v): %s", err, stdout) + } + addressed := envelope.Data[0].Addressed + if !addressed.BCCDisclosed || !addressed.Truncated { + t.Errorf("disclosed = %v truncated = %v, want both", addressed.BCCDisclosed, addressed.Truncated) + } + if len(addressed.BCC) != maxRetainedRecipients { + t.Errorf("bcc = %d addresses, want the bound of %d", len(addressed.BCC), maxRetainedRecipients) + } +} diff --git a/tests/smoke/compose_test.go b/tests/smoke/compose_test.go index ea482f15..95ae5cc8 100644 --- a/tests/smoke/compose_test.go +++ b/tests/smoke/compose_test.go @@ -88,6 +88,15 @@ func TestCompose(t *testing.T) { if len(verification.Recipients.To) != 1 || verification.Recipients.To[0] != "david@basecamp.com" { t.Errorf("verified recipients = %v, want exactly the address it was sent to", verification.Recipients.To) } + // Nothing was blind-copied. bcc_disclosed says whether the empty list is evidence of + // that or just HEY withholding the line — this is the one place a real server can + // tell us which it does, so record it rather than asserting a shape we are guessing + // at. A disclosed line, though, must be empty: nobody was BCC'd. + t.Logf("live BCC disclosure: bcc_disclosed=%v bcc=%v", + verification.Recipients.BCCDisclosed, verification.Recipients.BCC) + if verification.Recipients.BCCDisclosed && len(verification.Recipients.BCC) != 0 { + t.Errorf("bcc = %v, want nobody — this send named no BCC", verification.Recipients.BCC) + } if !verification.MatchesSent.Subject || !verification.MatchesSent.Body || !verification.MatchesSent.Recipients { t.Errorf("matches_sent = %+v, want every comparison to hold", verification.MatchesSent) } From e47d98bbe4279ecf26b34236663f27b0898a9447 Mon Sep 17 00:00:00 2001 From: jonata97 Date: Wed, 2 Sep 2026 15:42:31 -0300 Subject: [PATCH 3/5] fix(compose): treat a send whose outcome is unknown as ambiguous, not as a network error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/cli.md | 2 +- internal/cmd/compose.go | 6 +- internal/cmd/compose_dispatch_test.go | 362 ++++++++++++++++++++++++++ internal/cmd/compose_send.go | 104 +++++++- 4 files changed, 468 insertions(+), 6 deletions(-) create mode 100644 internal/cmd/compose_dispatch_test.go diff --git a/docs/cli.md b/docs/cli.md index 58c788d1..2cce40b8 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -263,7 +263,7 @@ Writing is Markdown too, everywhere text goes in: `-m`, `--content`, `--note`, p Drafts are the review-before-send lane: `hey compose --draft` (and `hey reply --draft`) saves instead of sending — recipients optional on a draft — and answers the draft's ID. `hey draft show` reads it back with the body as Markdown, `hey draft edit` revises it (each flag replaces its field; what is not flagged is kept, by reading the draft and resending the whole of it, since a revision is not a patch on HEY's side), `hey draft send` delivers through HEY's undo window, and `hey draft delete` trashes it. Scheduling a delivery is done in a HEY app for now — the API cannot yet name an exact instant — and a schedule set there survives CLI edits untouched. A draft prepared here is reviewed and sent from any HEY app, which is the workflow this is for: an agent writes, a person decides. -A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` says whether HEY served the BCC field at all, not whether anybody was on it: an explicitly empty field is disclosed, while an omitted or null field is not. So `bcc: []` with `bcc_disclosed: true` proves nobody was blind-copied; the same list with `bcc_disclosed: false` proves nothing. A send HEY accepted but named no message for is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. `--styled` output is unchanged: one line, `Message sent.` +A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` says whether HEY served the BCC field at all, not whether anybody was on it: an explicitly empty field is disclosed, while an omitted or null field is not. So `bcc: []` with `bcc_disclosed: true` proves nobody was blind-copied; the same list with `bcc_disclosed: false` proves nothing. A send whose outcome cannot be established is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. That covers a send HEY accepted but named no message for, a connection that died with the request already on it, an answer that could not be read, and any 5xx: none proves HEY did not act. Only a failure before the request went out, or a status that is itself a refusal — 401, 403, 404, 409, 422, 429 and 4xx generally — keeps its own code. When you see exit 8, read the thread back rather than sending again. `--styled` output is unchanged: one line, `Message sent.` `hey thread read --json` carries the same envelope per entry, off the same message record: `subject`, `sender` (the identity the message went out as, next to `creator`, who wrote it) and `addressed` with `to`, `cc`, `bcc` and `bcc_disclosed`, with the same field-presence meaning as on a send. Recipient lists are read from what HEY served, never inferred from a position or from the body, and are cut at a hundred addresses per line with `truncated` saying so. `--count` and `--ids-only` read no messages, so they carry no envelope rather than an invented one. diff --git a/internal/cmd/compose.go b/internal/cmd/compose.go index edca1178..f2a77c53 100644 --- a/internal/cmd/compose.go +++ b/internal/cmd/compose.go @@ -141,7 +141,7 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { response, sendErr = sendReply(ctx, replySDK, target.EntryID, target.ActingSenderID, sent.Subject, sent.Content, sent.To, sent.CC, sent.BCC) if sendErr != nil { - return apierr.FromSDK(sendErr) + return classifySendFailure(sendErr) } } else { to := parseAddresses(c.to) @@ -168,7 +168,7 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { var sendErr error response, sendErr = sendMessage(ctx, sdk, sent.Subject, sent.Content, sent.To, sent.CC, sent.BCC) if sendErr != nil { - return apierr.FromSDK(sendErr) + return classifySendFailure(sendErr) } } @@ -181,7 +181,7 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { if handleErr != nil { return apierr.ErrAmbiguousOutcome( fmt.Sprintf("the message may have been sent, but the response named no message to read back: %v", handleErr), - "Check the thread before sending again — a retry may deliver it twice.") + "Read the thread back before sending again — this endpoint has no idempotency key, so a retry may deliver the message twice.") } verification, readback := verifyComposedMessage(ctx, sendClient, handle.MessageID, sent) diff --git a/internal/cmd/compose_dispatch_test.go b/internal/cmd/compose_dispatch_test.go new file mode 100644 index 00000000..475e4741 --- /dev/null +++ b/internal/cmd/compose_dispatch_test.go @@ -0,0 +1,362 @@ +package cmd + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + + "github.com/basecamp/hey-cli/internal/apierr" + "github.com/basecamp/hey-cli/internal/output" +) + +// lostResponseServer answers everything a send needs and then, on the POST itself, +// reads the whole request body and drops the connection without writing a status — +// the shape of an accepted send whose answer never came back. sendPath selects which +// send is sabotaged, so the same server serves the new-message and the reply paths. +// +// The body is consumed on purpose: that is what makes this the dangerous case rather +// than a dial failure. HEY has the request. Whether it acted on it is unknowable from +// here, which is the whole point. +type lostResponse struct { + mu sync.Mutex + posts int + bodyBytes int + // truncate2xx writes a 200 and a Content-Length it does not honour, so the client + // sees the status and then loses the connection mid-body. + truncate2xx bool +} + +func (l *lostResponse) counts() (posts, bodyBytes int) { + l.mu.Lock() + defer l.mu.Unlock() + return l.posts, l.bodyBytes +} + +func lostResponseServer(t *testing.T, sendPath string) (*httptest.Server, *lostResponse) { + t.Helper() + state := &lostResponse{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == sendPath: + body, _ := io.ReadAll(r.Body) + state.mu.Lock() + state.posts++ + state.bodyBytes += len(body) + truncate := state.truncate2xx + state.mu.Unlock() + + hijacker, ok := w.(http.Hijacker) + if !ok { + t.Fatal("the test server cannot hijack, so the lost-response case cannot be staged") + } + conn, _, err := hijacker.Hijack() + if err != nil { + t.Fatalf("hijack: %v", err) + } + if truncate { + // A status the client will read, then a body that stops short of the + // length promised for it. + _, _ = io.WriteString(conn, "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: 64\r\n\r\n{\"id\":") + } + _ = conn.Close() + case strings.Contains(r.URL.Path, "identity"): + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"id":1,"accounts":[{"id":8,"status":"active"},{"id":9,"status":"active"}],"senders":[{"id":42,"account_id":9,"default":true},{"id":43,"account_id":8,"default":true}]}`) + case r.URL.Path == "/topics/7.json": + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"id":7,"account_id":9,"entries":[{"id":11},{"id":12}]}`) + case strings.HasSuffix(r.URL.Path, "/replies/new.json"): + w.Header().Set("Content-Type", "application/json") + http.Error(w, `{"message":"not found"}`, http.StatusNotFound) + case strings.HasPrefix(r.URL.Path, "/messages/"): + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, messageAddressedToJane) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.Error(w, "not found", http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + return server, state +} + +// assertAmbiguousSend is the contract an indeterminate send answers with: the ambiguous +// code, exit 8, a sentence saying the message may already have gone out, and a hint +// telling the caller to reconcile rather than retry. +func assertAmbiguousSend(t *testing.T, err error) { + t.Helper() + var cliErr *apierr.Error + if !errors.As(err, &cliErr) { + t.Fatalf("error = %v (%T), want the CLI's typed error", err, err) + } + if cliErr.Code != apierr.CodeAmbiguous { + t.Errorf("code = %q, want %q", cliErr.Code, apierr.CodeAmbiguous) + } + if got := output.ExitCodeFor(err); got != output.ExitAmbiguous { + t.Errorf("exit = %d, want %d", got, output.ExitAmbiguous) + } + if !strings.Contains(cliErr.Message, "may have been sent") { + t.Errorf("message = %q, want it to say the message may have gone out", cliErr.Message) + } + if cliErr.Hint == "" { + t.Fatal("an ambiguous send must carry a reconciliation hint") + } + if !strings.Contains(strings.ToLower(cliErr.Hint), "retry") { + t.Errorf("hint = %q, want it to warn against retrying", cliErr.Hint) + } +} + +// The reviewer's probe, committed. The server consumed one complete POST and then went +// away without answering. Reporting that as a network failure invites a retry, and a +// retry on an endpoint with no idempotency key delivers the message twice. +func TestComposeReportsAnAcceptedSendWithNoAnswerAsAmbiguous(t *testing.T) { + server, state := lostResponseServer(t, "/messages.json") + + _, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", + "-m", "Body.") + + assertAmbiguousSend(t, err) + posts, bodyBytes := state.counts() + if posts != 1 { + t.Errorf("the server saw %d POSTs, want exactly one — nothing here may retry a send", posts) + } + if bodyBytes == 0 { + t.Error("the server read no request body, so this is not the accepted-send case") + } +} + +// The reply path is the same non-idempotent send and gets the same answer. +func TestComposeReplyReportsAnAcceptedSendWithNoAnswerAsAmbiguous(t *testing.T) { + server, state := lostResponseServer(t, "/entries/12/replies.json") + + _, _, err := runCLIRaw(t, server, "--json", "--account", "8", "compose", + "--thread-id", "7", "-m", "Body.") + + assertAmbiguousSend(t, err) + posts, bodyBytes := state.counts() + if posts != 1 { + t.Errorf("the server saw %d POSTs, want exactly one", posts) + } + if bodyBytes == 0 { + t.Error("the server read no request body, so this is not the accepted-send case") + } +} + +// A 2xx the client saw and then lost mid-body is the strongest form of this: HEY said +// yes, and the answer naming what it made is gone. +func TestComposeReportsATruncatedSuccessResponseAsAmbiguous(t *testing.T) { + server, state := lostResponseServer(t, "/messages.json") + state.truncate2xx = true + + _, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", + "-m", "Body.") + + assertAmbiguousSend(t, err) + if posts, _ := state.counts(); posts != 1 { + t.Errorf("the server saw %d POSTs, want exactly one", posts) + } +} + +// classifySendFailure is the whole decision, so it is tested as one: everything that +// proves HEY refused the request before acting keeps its own taxonomy, and everything +// that cannot prove it is ambiguous. The default is ambiguous on purpose — an outcome +// this cannot classify is one it cannot rule out. +func TestClassifySendFailureKeepsProvableRejectionsAndAmbiguatesTheRest(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + // Provably never dispatched: the failure happened resolving the sender, which + // is a read that runs before the send. + { + name: "an auth failure before the send was dispatched", + err: notDispatched(hey.ErrAuth("Authentication failed")), + want: apierr.CodeAuth, + }, + { + name: "a network failure before the send was dispatched", + err: notDispatched(hey.ErrNetwork(errors.New("dial tcp: connection refused"))), + want: apierr.CodeNetwork, + }, + + // HEY answered, and its answer is authoritative that nothing was created. + // FromSDK carries an SDK usage error through as an API error, which is this + // repo's standing decision (see apierr's own tests). What matters here is that + // it is terminal: HEY was never asked to do anything. + {name: "the SDK's own usage refusal", err: hey.ErrUsage("a message needs a recipient"), want: apierr.CodeAPI}, + {name: "401", err: hey.ErrAuth("Authentication failed"), want: apierr.CodeAuth}, + {name: "403", err: hey.ErrForbiddenScope(), want: apierr.CodeForbidden}, + {name: "404", err: hey.ErrNotFound("Entry", "12"), want: apierr.CodeNotFound}, + {name: "429", err: hey.ErrRateLimit(30), want: apierr.CodeRateLimit}, + {name: "422", err: hey.ErrValidation("Subject can't be blank"), want: apierr.CodeValidation}, + {name: "409", err: hey.ErrConflict("already sent"), want: apierr.CodeConflict}, + {name: "a plain 400", err: hey.ErrAPI(400, "Bad request"), want: apierr.CodeAPI}, + // A 4xx the SDK did not give a code of its own keeps the API code and its + // status; the point is that it does not become an ambiguous send. + {name: "a plain 404 carried as an API error", err: hey.ErrAPI(404, "Not found"), want: apierr.CodeAPI}, + { + // The status is what it means, size or no size: HEY refused the request. + name: "an oversized 422 body still carries its status", + err: &hey.Error{Code: hey.CodeValidation, HTTPStatus: 422, Message: "invalid", Cause: fmt.Errorf("%w of 16 bytes", hey.ErrResponseTooLarge)}, + want: apierr.CodeValidation, + }, + + // Nothing here proves HEY did not act. + {name: "a transport failure", err: hey.ErrNetwork(errors.New("EOF")), want: apierr.CodeAmbiguous}, + {name: "500", err: hey.ErrAPI(500, "Server error (500)"), want: apierr.CodeAmbiguous}, + {name: "502", err: &hey.Error{Code: hey.CodeAPI, HTTPStatus: 502, Message: "Gateway error (502)", Retryable: true}, want: apierr.CodeAmbiguous}, + {name: "503", err: &hey.Error{Code: hey.CodeAPI, HTTPStatus: 503, Message: "Gateway error (503)", Retryable: true}, want: apierr.CodeAmbiguous}, + {name: "504", err: &hey.Error{Code: hey.CodeAPI, HTTPStatus: 504, Message: "Gateway error (504)", Retryable: true}, want: apierr.CodeAmbiguous}, + { + name: "a response that could not be read after HEY accepted it", + err: fmt.Errorf("failed to read response: %w", io.ErrUnexpectedEOF), + want: apierr.CodeAmbiguous, + }, + { + name: "a success body past the response cap", + err: fmt.Errorf("failed to read response: %w of 16777216 bytes", hey.ErrResponseTooLarge), + want: apierr.CodeAmbiguous, + }, + {name: "a deadline that passed mid-request", err: context.DeadlineExceeded, want: apierr.CodeAmbiguous}, + {name: "a cancelled request", err: context.Canceled, want: apierr.CodeAmbiguous}, + {name: "an error nothing here recognises", err: errors.New("something else entirely"), want: apierr.CodeAmbiguous}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := classifySendFailure(tt.err) + var cliErr *apierr.Error + if !errors.As(got, &cliErr) { + t.Fatalf("error = %v (%T), want the CLI's typed error", got, got) + } + if cliErr.Code != tt.want { + t.Fatalf("code = %q, want %q (message %q)", cliErr.Code, tt.want, cliErr.Message) + } + if tt.want == apierr.CodeAmbiguous { + assertAmbiguousSend(t, got) + return + } + // A rejection keeps its own words rather than being dressed up as a + // possible send. + if strings.Contains(cliErr.Message, "may have been sent") { + t.Errorf("message = %q, want a plain rejection", cliErr.Message) + } + }) + } +} + +// The taxonomy survives end to end, not only in the classifier: a status HEY answers +// with reaches the caller as the code that status means. +func TestComposeKeepsEachAnsweredStatusInItsOwnLane(t *testing.T) { + tests := []struct { + name string + status int + wantCode string + wantExit int + }{ + {name: "rate limited", status: http.StatusTooManyRequests, wantCode: apierr.CodeRateLimit, wantExit: output.ExitRateLimit}, + {name: "forbidden", status: http.StatusForbidden, wantCode: apierr.CodeForbidden, wantExit: output.ExitForbidden}, + {name: "not found", status: http.StatusNotFound, wantCode: apierr.CodeNotFound, wantExit: output.ExitNotFound}, + {name: "unprocessable", status: http.StatusUnprocessableEntity, wantCode: apierr.CodeAPI, wantExit: output.ExitAPI}, + {name: "server error", status: http.StatusInternalServerError, wantCode: apierr.CodeAmbiguous, wantExit: output.ExitAmbiguous}, + {name: "bad gateway", status: http.StatusBadGateway, wantCode: apierr.CodeAmbiguous, wantExit: output.ExitAmbiguous}, + {name: "service unavailable", status: http.StatusServiceUnavailable, wantCode: apierr.CodeAmbiguous, wantExit: output.ExitAmbiguous}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server, sent := composeSendServer(t) + sent.SendStatus = tt.status + sent.SendLocation = "" + + _, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", + "-m", "Body.") + + var cliErr *apierr.Error + if !errors.As(err, &cliErr) { + t.Fatalf("error = %v, want the CLI's typed error", err) + } + if cliErr.Code != tt.wantCode { + t.Errorf("code = %q, want %q (message %q)", cliErr.Code, tt.wantCode, cliErr.Message) + } + if got := output.ExitCodeFor(err); got != tt.wantExit { + t.Errorf("exit = %d, want %d", got, tt.wantExit) + } + if sent.Reads != 0 { + t.Errorf("readbacks = %d, want none — no send was confirmed", sent.Reads) + } + }) + } +} + +// A failure before the send goes out keeps its own taxonomy, and the proof is that the +// server never saw a POST at all. +func TestComposeKeepsAPreDispatchFailureOutOfTheAmbiguousLane(t *testing.T) { + var posts int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + posts++ + t.Errorf("nothing should have been posted: %s", r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + http.Error(w, `{"message":"unauthorized"}`, http.StatusUnauthorized) + })) + t.Cleanup(server.Close) + + _, _, err := runCLIRaw(t, server, "--json", "compose", + "--to", "alice@example.com", "--subject", "Inovo Customer Update — Week 12", + "-m", "Body.") + + var cliErr *apierr.Error + if !errors.As(err, &cliErr) { + t.Fatalf("error = %v, want the CLI's typed error", err) + } + if cliErr.Code != apierr.CodeAuth { + t.Errorf("code = %q, want %q — the sender could not be resolved, so nothing was sent", + cliErr.Code, apierr.CodeAuth) + } + if posts != 0 { + t.Errorf("the server saw %d POSTs, want none", posts) + } +} + +// The ambiguity belongs to the send, not to the CLI. A read that cannot reach HEY — +// the same transport failure that makes a send indeterminate — keeps whatever taxonomy +// it always had, because reading again costs nothing and delivers nothing. The +// classifier is wired to the two send call sites and nowhere else, and this is what +// says so. +func TestAFailedReadIsNeverAnAmbiguousSend(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + server.Close() // nothing is listening, so every request fails to connect + + _, _, err := runCLIRaw(t, server, "--json", "thread", "read", "7") + if err == nil { + t.Fatal("a read against nothing must fail") + } + + var cliErr *apierr.Error + if !errors.As(err, &cliErr) { + t.Fatalf("error = %v, want the CLI's typed error", err) + } + if cliErr.Code == apierr.CodeAmbiguous { + t.Errorf("code = %q, want a read to keep its own taxonomy", cliErr.Code) + } + if got := output.ExitCodeFor(err); got == output.ExitAmbiguous { + t.Errorf("exit = %d, want anything but the ambiguous-send code", got) + } + if strings.Contains(cliErr.Message, "may have been sent") { + t.Errorf("message = %q, want nothing about sending", cliErr.Message) + } +} diff --git a/internal/cmd/compose_send.go b/internal/cmd/compose_send.go index 8df8f725..a259767a 100644 --- a/internal/cmd/compose_send.go +++ b/internal/cmd/compose_send.go @@ -2,10 +2,13 @@ package cmd import ( "context" + "errors" "fmt" "github.com/basecamp/hey-sdk/go/pkg/generated" hey "github.com/basecamp/hey-sdk/go/pkg/hey" + + "github.com/basecamp/hey-cli/internal/apierr" ) // A send is posted here rather than through MessagesService.Create and @@ -27,7 +30,7 @@ import ( func sendMessage(ctx context.Context, client *hey.Client, subject, content string, to, cc, bcc []string) (*hey.Response, error) { senderID, err := actingSenderID(ctx, client, 0) if err != nil { - return nil, err + return nil, notDispatched(err) } body := generated.CreateMessageRequestContent{ ActingSenderId: senderID, @@ -45,7 +48,7 @@ func sendMessage(ctx context.Context, client *hey.Client, subject, content strin func sendReply(ctx context.Context, client *hey.Client, entryID, actingSender int64, subject, content string, to, cc, bcc []string) (*hey.Response, error) { senderID, err := actingSenderID(ctx, client, actingSender) if err != nil { - return nil, err + return nil, notDispatched(err) } body := generated.CreateReplyRequestContent{ ActingSenderId: senderID, @@ -65,3 +68,100 @@ func actingSenderID(ctx context.Context, client *hey.Client, chosen int64) (int6 } return client.DefaultSenderID(ctx) } + +// --- Classifying how a send failed --- + +// errNotDispatched marks a failure that happened before the send's own request was put +// on the wire — resolving the acting sender, which is a read. Nothing was sent, so such +// a failure keeps whatever taxonomy it came with instead of becoming an ambiguous +// outcome. It is a marker rather than a replacement: the original error travels inside +// it, so errors.As still finds the *hey.Error underneath. +var errNotDispatched = errors.New("the send was not dispatched") + +func notDispatched(err error) error { + return fmt.Errorf("%w: %w", errNotDispatched, err) +} + +// classifySendFailure decides what a failed send means to whoever has to act on it. +// +// A send is not idempotent and this endpoint carries no idempotency key, so the only +// question that matters is whether the failure proves HEY did not act. Two outcomes +// prove it: the request was never dispatched, or HEY answered with a status that is +// itself a refusal. Everything else — a connection that died with the request already +// on it, a 5xx, an answer that could not be read, an error nothing here recognises — +// leaves a message that may already be in somebody's inbox. +// +// So the ambiguous case is the default, not a special case. The failure mode of getting +// this backwards is a caller reading `network` off a send HEY completed, retrying, and +// delivering twice; the failure mode of getting it wrong this way is a caller going to +// look at a thread that turns out to be empty. Only one of those is recoverable. +// +// This is deliberately not applied to reads. `hey thread read` failing on the network +// is a network failure: reading again costs nothing and delivers nothing. The +// classification belongs to the two send call sites in compose.go and nowhere else. +// +// 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, so that +// retry cannot duplicate a delivery. +func classifySendFailure(err error) error { + if err == nil { + return nil + } + // Never on the wire, so whatever went wrong is still just what went wrong. + if errors.Is(err, errNotDispatched) { + return apierr.FromSDK(err) + } + if rejected(err) { + return apierr.FromSDK(err) + } + return apierr.ErrAmbiguousOutcome( + fmt.Sprintf("the message may have been sent: %s", indeterminateReason(err)), + "Read the thread back before sending again — this endpoint has no idempotency key, so a retry may deliver the message twice.") +} + +// rejected reports whether the failure is HEY declining the request, which is the one +// thing that proves no message was created. +// +// It works from the code and the status rather than from the sentence, and it is an +// allowlist: a failure that does not match is ambiguous. The SDK does not set an HTTP +// status on every refusal it builds — ErrAuth and ErrNotFound carry none — so the code +// is checked first and the status second, and a bare error (a response that could not +// be read, say) matches neither. +func rejected(err error) bool { + var sdkErr *hey.Error + if !errors.As(err, &sdkErr) { + return false + } + switch sdkErr.Code { + case hey.CodeUsage, hey.CodeAuth, hey.CodeForbidden, hey.CodeNotFound, + hey.CodeRateLimit, hey.CodeValidation, hey.CodeConflict: + // Each of these is HEY refusing before it acts: a malformed request, a + // credential it would not take, a scope it would not allow, a route or entry + // that is not there, a limit it turned the request away at, contents it would + // not accept, or a state it conflicts with. + return true + } + // Anything else HEY put a 4xx on is the same kind of answer: the request was bad + // and was not carried out. A 5xx is not — see the mutation contract in the SDK, + // which is why it does not retry one either. + return sdkErr.HTTPStatus >= 400 && sdkErr.HTTPStatus <= 499 +} + +// indeterminateReason says which way the send became unknowable, in the CLI's own +// words. It never quotes the server: what is useful here is which of the three shapes +// this was, and a reader who wants the detail has the thread to go and look at. +func indeterminateReason(err error) string { + var sdkErr *hey.Error + switch { + case errors.Is(err, hey.ErrResponseTooLarge): + return "HEY answered, but its answer was past the size this reads and could not be taken in" + case errors.As(err, &sdkErr) && sdkErr.HTTPStatus >= 500: + return fmt.Sprintf("HEY answered %d, which does not say whether it acted on the request", sdkErr.HTTPStatus) + case errors.As(err, &sdkErr) && sdkErr.Code == hey.CodeNetwork: + return "the connection failed with the request already sent, so HEY's answer never arrived" + case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded): + return "the request was given up on before HEY answered" + default: + return "HEY's answer could not be read, so what it did with the request is unknown" + } +} From b5bc1cf12ac4631437849bd539198cb95b36e113 Mon Sep 17 00:00:00 2001 From: jonata97 Date: Fri, 4 Sep 2026 16:14:24 -0300 Subject: [PATCH 4/5] fix(compose): make delivery handle verification fail closed --- .surface | 1 + README.md | 1 + docs/cli.md | 5 +- internal/cmd/addressed.go | 8 +- internal/cmd/compose.go | 52 +- internal/cmd/compose_verifiable.go | 156 ++++++ internal/cmd/compose_verifiable_test.go | 647 ++++++++++++++++++++++++ internal/cmd/compose_verify.go | 18 +- internal/cmd/root.go | 4 +- internal/cmd/root_test.go | 13 + skills/hey/SKILL.md | 3 + 11 files changed, 896 insertions(+), 12 deletions(-) create mode 100644 internal/cmd/compose_verifiable.go create mode 100644 internal/cmd/compose_verifiable_test.go diff --git a/.surface b/.surface index 7d35c367..18be3b99 100644 --- a/.surface +++ b/.surface @@ -107,6 +107,7 @@ hey compose --message-html hey compose --subject hey compose --thread-id hey compose --to +hey compose --verifiable hey config hey config set hey config show diff --git a/README.md b/README.md index 0c27d406..0752c512 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,7 @@ hey box view imbox # threads in a box hey thread read 12345 # a whole thread, as Markdown hey reply 12345 -m "Friday works for me." hey compose --to alice@example.com --subject "Lunch?" -m "Thursday at noon?" +hey compose --to alice@example.com --subject "Customer update" -m "Done." --verifiable hey search --from jane@example.com --date last_30_days hey screener list # first-time senders waiting on you hey event add "Design review" --starts-on 2026-09-02 --start-time 14:00 diff --git a/docs/cli.md b/docs/cli.md index 2cce40b8..82c61abe 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -233,6 +233,7 @@ hey compose --to alice@example.com --cc bob@example.com --bcc carol@example.org hey compose --to alice@example.com --subject "Sprint recap" -m "We **shipped** the pagination fix." hey compose --to alice@example.com --subject "Newsletter draft" --message-html "

March

What we shipped.

" hey compose --subject "Board update" -m "Numbers to follow." --draft # save a draft instead of sending +hey compose --to alice@example.com --subject "Customer update" -m "The migration is complete." --verifiable # known-ID send with exact readback hey reply 123 -m "Drafting a longer answer." --draft # save a reply draft hey draft list # list drafts (--all and --page follow HEY's cursor) hey draft show 12345 # read a draft back @@ -263,7 +264,9 @@ Writing is Markdown too, everywhere text goes in: `-m`, `--content`, `--note`, p Drafts are the review-before-send lane: `hey compose --draft` (and `hey reply --draft`) saves instead of sending — recipients optional on a draft — and answers the draft's ID. `hey draft show` reads it back with the body as Markdown, `hey draft edit` revises it (each flag replaces its field; what is not flagged is kept, by reading the draft and resending the whole of it, since a revision is not a patch on HEY's side), `hey draft send` delivers through HEY's undo window, and `hey draft delete` trashes it. Scheduling a delivery is done in a HEY app for now — the API cannot yet name an exact instant — and a schedule set there survives CLI edits untouched. A draft prepared here is reviewed and sent from any HEY app, which is the workflow this is for: an agent writes, a person decides. -A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` says whether HEY served the BCC field at all, not whether anybody was on it: an explicitly empty field is disclosed, while an omitted or null field is not. So `bcc: []` with `bcc_disclosed: true` proves nobody was blind-copied; the same list with `bcc_disclosed: false` proves nothing. A send whose outcome cannot be established is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. That covers a send HEY accepted but named no message for, a connection that died with the request already on it, an answer that could not be read, and any 5xx: none proves HEY did not act. Only a failure before the request went out, or a status that is itself a refusal — 401, 403, 404, 409, 422, 429 and 4xx generally — keeps its own code. When you see exit 8, read the thread back rather than sending again. `--styled` output is unchanged: one line, `Message sent.` +A send answers what it created. `hey compose --json` reports `message_id` — the entry HEY named in its response — `topic_id` and `app_url` where they are known, and a `verification` object holding what reading that message back showed: the `subject` HEY stored, the `sender` address it went out as, the `recipients` it reached, the body as canonical Markdown with its `body_markdown_sha256`, and `matches_sent`, which compares each of those with what was asked for. `status` is `verified` when everything comparable matches, `mismatch` when the message exists but differs — never a reason to send again — and `unverified` when the message could not be read back, with a `reason`. `recipients.bcc_disclosed` says whether HEY served the BCC field at all, not whether anybody was on it: an explicitly empty field is disclosed, while an omitted or null field is not. So `bcc: []` with `bcc_disclosed: true` proves nobody was blind-copied; the same list with `bcc_disclosed: false` proves nothing. A send whose outcome cannot be established is neither reported as success nor retried: it exits `ambiguous` (exit 8) saying the message may have been sent, because a retry on this endpoint — which carries no idempotency key — can deliver it twice. That covers a send HEY accepted but named no message for, a connection that died with the request already on it, an answer that could not be read, and any 5xx: none proves HEY did not act. Only a failure before the request went out, or a status that is itself a refusal — 401, 403, 404, 409, 422, 429 and 4xx generally — keeps its own code. When you see exit 8, read the thread back rather than sending again. `--styled` keeps the ordinary one-line confirmation. + +For automation that requires a reconcilable send, `hey compose --verifiable` first saves a draft to obtain a stable `message_id`, reads that exact draft back through its edit endpoint, sends it exactly once only after the draft matches, and then reads only that ID back as a delivered message. It succeeds only when the delivery readback carries that ID plus an exact `/topics/` followed by a positive numeric ID as its URL path, and an explicitly present `sender` ID, subject, To, CC, explicitly disclosed BCC (including an explicit empty list), and body all match. Missing or mismatched proof exits `ambiguous` (exit 8) with the known ID and bounded boolean reconciliation checks; it never searches by subject or time and never retries the send. Its Markdown input must contain no raw HTML or Action Text attachment markup, and `--verifiable` cannot be combined with `--draft`, `--thread-id`, `--attach`, or `--message-html`. `hey thread read --json` carries the same envelope per entry, off the same message record: `subject`, `sender` (the identity the message went out as, next to `creator`, who wrote it) and `addressed` with `to`, `cc`, `bcc` and `bcc_disclosed`, with the same field-presence meaning as on a send. Recipient lists are read from what HEY served, never inferred from a position or from the body, and are cut at a hundred addresses per line with `truncated` saying so. `--count` and `--ids-only` read no messages, so they carry no envelope rather than an invented one. diff --git a/internal/cmd/addressed.go b/internal/cmd/addressed.go index c8e2f3b9..12b80cb3 100644 --- a/internal/cmd/addressed.go +++ b/internal/cmd/addressed.go @@ -59,14 +59,16 @@ func addressedFrom(addressed generated.Addressed) addressedEnvelope { } } -// boundedEmails answers the contacts' addresses, dropping blanks and stopping at -// maxRetainedRecipients. The addresses are HEY's own, so they are carried verbatim; +// boundedEmails answers the contacts' addresses, marking the envelope incomplete when a +// contact has no address or when the bound is reached. The addresses are HEY's own, so +// they are carried verbatim; // whatever prints one sanitizes it there, as every other read of somebody else's text // does. func boundedEmails(contacts []generated.Contact) (emails []string, truncated bool) { emails = make([]string, 0, min(len(contacts), maxRetainedRecipients)) for _, contact := range contacts { if contact.EmailAddress == "" { + truncated = true continue } if len(emails) == maxRetainedRecipients { @@ -74,5 +76,5 @@ func boundedEmails(contacts []generated.Contact) (emails []string, truncated boo } emails = append(emails, contact.EmailAddress) } - return emails, false + return emails, truncated } diff --git a/internal/cmd/compose.go b/internal/cmd/compose.go index f2a77c53..d06722a4 100644 --- a/internal/cmd/compose.go +++ b/internal/cmd/compose.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/spf13/cobra" + xhtml "golang.org/x/net/html" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -26,6 +27,7 @@ type composeCommand struct { threadID string attachments []string draft bool + verifiable bool } func newComposeCommand() *composeCommand { @@ -34,13 +36,13 @@ func newComposeCommand() *composeCommand { Use: "compose", Short: "Write and send a new email", Annotations: map[string]string{ - "agent_notes": "Starts a new thread with --to (optionally --cc/--bcc), which requires --subject, or replies to an existing one with --thread-id, which does not. Repeatable --attach files are uploaded before sending and can be sent without body text. The body is Markdown; use --message-html to send raw HTML instead. --draft saves instead of sending — recipients become optional — and answers the draft ID for hey draft show/edit/send/delete.", + "agent_notes": "Starts a new thread with --to (optionally --cc/--bcc), which requires --subject, or replies to an existing one with --thread-id, which does not. Repeatable --attach files are uploaded before sending and can be sent without body text. The body is Markdown; use --message-html to send raw HTML instead. --draft saves instead of sending — recipients become optional — and answers the draft ID for hey draft show/edit/send/delete. For automation that must reconcile a lost send response without searching or retrying, --verifiable saves and reads back an exact Markdown-only draft to obtain its stable message ID, sends that draft once, and reports success only after reading the delivered message at that exact ID.", }, Example: ` hey compose --to alice@example.com --subject "Lunch plans" -m "Are you free Friday?" hey compose --to alice@example.com --cc bob@example.com --bcc carol@example.org --subject "Kitchen remodel timeline" -m "Cabinets land the week of the 14th." hey compose --to alice@example.com --subject "Q3 revenue report" -m "The numbers are attached." --attach ./report.pdf hey compose --thread-id 12345 -m "Confirmed — see you then." --attach ./diagram.png - hey compose --to alice@example.com --subject "Sprint recap" -m "We **shipped** the pagination fix." + hey compose --to alice@example.com --subject "Sprint recap" -m "We **shipped** the pagination fix." --verifiable hey compose --to alice@example.com --subject "Newsletter draft" --message-html "

March

What we shipped.

" echo "Notes from the offsite" | hey compose --to bob@example.com --subject "Offsite recap" hey compose --subject "Board update" -m "Numbers to follow." --draft # save a draft; add recipients later`, @@ -56,6 +58,7 @@ func newComposeCommand() *composeCommand { composeCommand.cmd.Flags().StringVar(&composeCommand.threadID, "thread-id", "", "Reply to this thread instead of starting a new one") composeCommand.cmd.Flags().StringArrayVar(&composeCommand.attachments, "attach", nil, "File to attach (repeatable)") composeCommand.cmd.Flags().BoolVar(&composeCommand.draft, "draft", false, "Save as a draft instead of sending") + composeCommand.cmd.Flags().BoolVar(&composeCommand.verifiable, "verifiable", false, "Create a known-ID draft, send it once, and require exact readback") composeCommand.cmd.MarkFlagsMutuallyExclusive("message", "message-html") return composeCommand @@ -65,6 +68,18 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { if err := requireAuth(); err != nil { return err } + if c.verifiable { + switch { + case c.draft: + return apierr.ErrUsage("--verifiable cannot be combined with --draft") + case c.threadID != "": + return apierr.ErrUsage("--verifiable cannot be combined with --thread-id") + case c.messageHTML != "": + return apierr.ErrUsage("--verifiable cannot be combined with --message-html") + case len(c.attachments) > 0: + return apierr.ErrUsage("--verifiable cannot be combined with --attach") + } + } // A reply carries the thread's subject, so only a new message needs one. if c.subject == "" && c.threadID == "" { @@ -93,6 +108,9 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { return apierr.ErrUsage("empty message, aborting") } } + if c.verifiable && containsHTMLMarkup(markdownMessage) { + return apierr.ErrUsage("--verifiable Markdown cannot contain raw HTML or attachment markup") + } message = htmlutil.FromMarkdown(markdownMessage) } @@ -165,6 +183,13 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { return writeDraftSaved(cmd, draftID, len(c.attachments)) } sent = composeSent{Subject: c.subject, Content: messageWithAttachments, To: to, CC: cc, BCC: bcc} + if c.verifiable { + result, verifyErr := composeVerifiably(ctx, sdk, sent) + if verifyErr != nil { + return verifyErr + } + return writeComposedMessage(cmd, result, len(c.attachments)) + } var sendErr error response, sendErr = sendMessage(ctx, sdk, sent.Subject, sent.Content, sent.To, sent.CC, sent.BCC) if sendErr != nil { @@ -186,8 +211,11 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { verification, readback := verifyComposedMessage(ctx, sendClient, handle.MessageID, sent) result := composeResultFor(handle, verification, readback) + return writeComposedMessage(cmd, result, len(c.attachments)) +} - summary := sentWithAttachmentsSummary("Message sent", len(c.attachments)) +func writeComposedMessage(cmd *cobra.Command, result composeResult, attachments int) error { + summary := sentWithAttachmentsSummary("Message sent", attachments) if result.TopicID != 0 { return writeMutation(cmd, summary, result, output.WithBreadcrumbs( output.Breadcrumb{ @@ -227,3 +255,21 @@ func parseAddresses(s string) []string { } return addrs } + +// containsHTMLMarkup keeps verifiable sends inside a canonical Markdown subset. Raw HTML +// and Action Text attachment elements lose attributes during Markdown readback, so accepting +// them would let distinct wire bodies compare equal. +func containsHTMLMarkup(markdown string) bool { + tokens := xhtml.NewTokenizer(strings.NewReader(markdown)) + for { + switch tokens.Next() { + case xhtml.StartTagToken, xhtml.EndTagToken, xhtml.SelfClosingTagToken, + xhtml.CommentToken, xhtml.DoctypeToken: + return true + case xhtml.TextToken: + continue + case xhtml.ErrorToken: + return false + } + } +} diff --git a/internal/cmd/compose_verifiable.go b/internal/cmd/compose_verifiable.go new file mode 100644 index 00000000..012b2f0d --- /dev/null +++ b/internal/cmd/compose_verifiable.go @@ -0,0 +1,156 @@ +package cmd + +import ( + "context" + "net/url" + "strconv" + "strings" + + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + + "github.com/basecamp/hey-cli/internal/apierr" +) + +type verifiableReadbackChecks struct { + Readable bool `json:"readable"` + MessageID bool `json:"message_id"` + DeliveryTopic bool `json:"delivery_topic"` + VerificationStatus bool `json:"verification_status"` + Sender bool `json:"sender"` + Subject bool `json:"subject"` + To bool `json:"to"` + CC bool `json:"cc"` + BCCDisclosed bool `json:"bcc_disclosed"` + BCC bool `json:"bcc"` + Body bool `json:"body"` +} + +func (c verifiableReadbackChecks) exact() bool { + return c.Readable && c.MessageID && c.DeliveryTopic && c.VerificationStatus && c.Sender && c.Subject && + c.To && c.CC && c.BCCDisclosed && c.BCC && c.Body +} + +// composeVerifiably creates an unsent message first so the delivery has an identifier +// before it begins. The SDK contract defines CreateDraft's answer as the entry ID used +// by SendDraft and Messages.Get; SendDraft revises that same /messages/{id} resource in +// place. That gives an ambiguous send one safe reconciliation query: this ID, never a +// subject/time search and never a second send. +func composeVerifiably(ctx context.Context, client *hey.Client, sent composeSent) (composeResult, error) { + senderID, err := actingSenderID(ctx, client, 0) + if err != nil { + return composeResult{}, notDispatched(err) + } + + draft := hey.DraftContent{ + Subject: sent.Subject, Content: sent.Content, + To: sent.To, CC: sent.CC, BCC: sent.BCC, + ActingSenderID: senderID, + } + draftID, err := client.Messages().CreateDraft(ctx, draft) + if err != nil { + // No delivery request has happened. A missing draft handle may leave an unsent + // draft behind, but it cannot have delivered this message. + return composeResult{}, apierr.FromSDK(err) + } + if err := verifyCreatedDraft(ctx, client, draftID, sent); err != nil { + return composeResult{}, notDispatched(err) + } + + if sendErr := client.Messages().SendDraft(ctx, draftID, draft); sendErr != nil { + classified := classifySendFailure(sendErr) + if apierr.AsError(classified).Code != apierr.CodeAmbiguous { + return composeResult{}, classified + } + } + + verification, readback := verifyComposedMessage(ctx, client, draftID, sent) + result := composeResultFor(composeHandle{MessageID: draftID}, verification, readback) + checks := verifiableReadbackChecks{Readable: readback != nil} + if readback != nil { + checks.MessageID = readback.Id == draftID + checks.DeliveryTopic = deliveredTopicID(readback.Url) > 0 + checks.VerificationStatus = verification.Status == verificationVerified + checks.Sender = readback.Sender.Id > 0 && readback.Sender.Id == senderID + if verification.MatchesSent != nil { + checks.Subject = verification.MatchesSent.Subject + checks.Body = verification.MatchesSent.Body + } + if verification.Recipients != nil { + recipients := verification.Recipients + untruncated := !recipients.Truncated + checks.To = untruncated && sameAddresses(sent.To, recipients.To) + checks.CC = untruncated && sameAddresses(sent.CC, recipients.CC) + checks.BCCDisclosed = recipients.BCCDisclosed + checks.BCC = untruncated && recipients.BCCDisclosed && sameAddresses(sent.BCC, recipients.BCC) + } + } + if checks.exact() { + return result, nil + } + return composeResult{}, unknownVerifiableCompose(draftID, checks) +} + +// verifyCreatedDraft proves the identifier returned by CreateDraft names the exact draft +// just saved before the one delivery request is allowed. The SDK returns only a numeric ID +// parsed from Location; GetEdit binds that ID back to a draft resource and its full content. +func verifyCreatedDraft(ctx context.Context, client *hey.Client, draftID int64, sent composeSent) error { + draft, err := client.Messages().GetEdit(ctx, draftID) + if err != nil { + return apierr.FromSDK(err) + } + if draft == nil { + return apierr.ErrAPI(0, "saved draft readback was empty; delivery was not attempted") + } + recipients := addressedFrom(draft.Addressed) + 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 !exact { + return apierr.ErrAPI(0, "saved draft did not read back exactly; delivery was not attempted") + } + return nil +} + +// deliveredTopicID accepts a topic only from the URL path. Query strings often carry a +// return_to=/topics/... navigation hint even while the resource itself is still a draft. +func deliveredTopicID(raw string) int64 { + parsed, err := url.Parse(raw) + if err != nil { + return 0 + } + const prefix = "/topics/" + path := parsed.EscapedPath() + if !strings.HasPrefix(path, prefix) { + return 0 + } + rawID := strings.TrimPrefix(path, prefix) + if rawID == "" || rawID[0] < '1' || rawID[0] > '9' { + return 0 + } + for _, digit := range rawID[1:] { + if digit < '0' || digit > '9' { + return 0 + } + } + id, err := strconv.ParseInt(rawID, 10, 64) + if err != nil || id <= 0 { + return 0 + } + return id +} + +func unknownVerifiableCompose(draftID int64, checks verifiableReadbackChecks) error { + err := apierr.ErrAmbiguousOutcome( + "the message may have been sent, but its known message ID did not produce an exact readback", + "Do not retry: reconcile message_id from this error with HEY; the one delivery request may already have succeeded.") + err.Meta = map[string]any{ + "message_id": draftID, + "reconciliation": checks, + } + return err +} diff --git a/internal/cmd/compose_verifiable_test.go b/internal/cmd/compose_verifiable_test.go new file mode 100644 index 00000000..f4dabd9b --- /dev/null +++ b/internal/cmd/compose_verifiable_test.go @@ -0,0 +1,647 @@ +package cmd + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/basecamp/hey-cli/internal/apierr" +) + +type verifiableComposeState struct { + draftWrites int + draftReads int + sendWrites int + messageReads int + requests []string + accountScopes map[string][]string + + sendStatus int + readStatus int + readJSON string + + draftBody map[string]any + sendBody map[string]any +} + +type verifiableReadback struct { + messageID int64 + url string + senderID int64 + subject string + content string + to []string + cc []string + bcc []string + bccDisclosed bool +} + +func exactVerifiableReadback() verifiableReadback { + return verifiableReadback{ + messageID: 12345, + url: "https://app.hey.com/topics/7742", + senderID: 42, + subject: "Inovo Customer Update — Week 12", + content: "

Body.

", + to: []string{"alice@example.com"}, + cc: []string{"bob@example.com"}, + bcc: []string{"carol@example.org"}, + bccDisclosed: true, + } +} + +func encodeVerifiableReadback(t *testing.T, readback verifiableReadback) string { + t.Helper() + addressed := map[string]any{ + "directly": contactsFor(readback.to), + "copied": contactsFor(readback.cc), + } + if readback.bccDisclosed { + addressed["blindcopied"] = contactsFor(readback.bcc) + } + payload, err := json.Marshal(map[string]any{ + "id": readback.messageID, + "subject": readback.subject, + "content": readback.content, + "url": readback.url, + "sender": map[string]any{ + "id": readback.senderID, "name": "Nova Desk", "email_address": "nova@example.com", + }, + "addressed": addressed, + }) + if err != nil { + t.Fatalf("encode verifiable readback: %v", err) + } + return string(payload) +} + +func encodeCreatedDraft(t *testing.T, body map[string]any) string { + t.Helper() + message, _ := body["message"].(map[string]any) + entry, _ := body["entry"].(map[string]any) + addressed, _ := entry["addressed"].(map[string]any) + contacts := func(raw any) []map[string]any { + values, _ := raw.([]any) + out := make([]map[string]any, 0, len(values)) + for i, value := range values { + address, _ := value.(string) + out = append(out, map[string]any{"id": i + 1, "email_address": address}) + } + return out + } + payload, err := json.Marshal(map[string]any{ + "id": 12345, + "subject": message["subject"], + "content": message["content"], + "sender": map[string]any{"id": body["acting_sender_id"]}, + "addressed": map[string]any{ + "directly": contacts(addressed["directly"]), + "copied": contacts(addressed["copied"]), + "blindcopied": contacts(addressed["blindcopied"]), + }, + }) + if err != nil { + t.Fatalf("encode created draft: %v", err) + } + return string(payload) +} + +func verifiableComposeServer(t *testing.T, state *verifiableComposeState) *httptest.Server { + t.Helper() + if state.accountScopes == nil { + state.accountScopes = make(map[string][]string) + } + defaultReadback := encodeVerifiableReadback(t, exactVerifiableReadback()) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + state.requests = append(state.requests, r.Method+" "+r.URL.Path) + state.accountScopes[r.Method+" "+r.URL.Path] = append( + state.accountScopes[r.Method+" "+r.URL.Path], r.URL.Query().Get("filtered_account_id")) + + switch { + case r.Method == http.MethodGet && r.URL.Path == "/identity.json": + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"id":1,"accounts":[{"id":840304,"status":"active"}],"senders":[{"id":42,"account_id":840304,"default":true}]}`) + case r.Method == http.MethodPost && r.URL.Path == "/messages.json": + state.draftWrites++ + _ = json.NewDecoder(r.Body).Decode(&state.draftBody) + w.Header().Set("Location", "https://app.hey.com/messages/12345") + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && r.URL.Path == "/messages/12345/edit.json": + state.draftReads++ + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, encodeCreatedDraft(t, state.draftBody)) + case r.Method == http.MethodPut && r.URL.Path == "/messages/12345.json": + state.sendWrites++ + _ = json.NewDecoder(r.Body).Decode(&state.sendBody) + status := state.sendStatus + if status == 0 { + status = http.StatusOK + } + w.WriteHeader(status) + case r.Method == http.MethodGet && r.URL.Path == "/messages/12345.json": + state.messageReads++ + status := state.readStatus + if status == 0 { + status = http.StatusOK + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + if state.readJSON != "" { + fmt.Fprint(w, state.readJSON) + return + } + fmt.Fprint(w, defaultReadback) + default: + t.Errorf("unexpected request (no search or discovery is allowed): %s %s", r.Method, r.URL.RequestURI()) + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + return server +} + +func runVerifiableCompose(t *testing.T, server *httptest.Server) (stdout, stderr string, err error) { + t.Helper() + return runCLIRaw(t, server, "--json", "--account", "840304", "compose", "--verifiable", + "--to", "alice@example.com", "--cc", "bob@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") +} + +func assertVerifiableRequestLedger(t *testing.T, state *verifiableComposeState) { + t.Helper() + if state.draftWrites != 1 || state.draftReads != 1 || state.sendWrites != 1 || state.messageReads != 1 { + t.Fatalf("draft writes/reads = %d/%d sends = %d message reads = %d, want 1/1/1/1", + state.draftWrites, state.draftReads, state.sendWrites, state.messageReads) + } + if got := strings.Join(state.requests, ", "); got != "GET /identity.json, POST /messages.json, GET /messages/12345/edit.json, PUT /messages/12345.json, GET /messages/12345.json" { + t.Fatalf("requests = %s, want only identity, known draft create, one draft send, and exact message read", got) + } + for route, scopes := range state.accountScopes { + for _, scope := range scopes { + if route == "GET /identity.json" { + if scope != "" { + t.Errorf("identity scope = %q, want unscoped account validation", scope) + } + continue + } + if scope != "840304" { + t.Errorf("%s scope = %q, want account 840304", route, scope) + } + } + } +} + +func allVerifiableChecks(value bool) map[string]bool { + return map[string]bool{ + "readable": value, + "message_id": value, + "delivery_topic": value, + "verification_status": value, + "sender": value, + "subject": value, + "to": value, + "cc": value, + "bcc_disclosed": value, + "bcc": value, + "body": value, + } +} + +func verifiableChecksWith(overrides map[string]bool) map[string]bool { + checks := allVerifiableChecks(true) + for name, value := range overrides { + checks[name] = value + } + return checks +} + +func assertUnknownVerifiableCompose(t *testing.T, stdout string, err error, state *verifiableComposeState, wantChecks map[string]bool) { + t.Helper() + if stdout != "" { + t.Errorf("stdout = %q, want no success envelope", stdout) + } + assertAmbiguousSend(t, err) + + var cliErr *apierr.Error + if !errors.As(err, &cliErr) { + t.Fatalf("error = %v, want the CLI's typed error", err) + } + if len(cliErr.Meta) != 2 { + t.Errorf("metadata keys = %d (%#v), want only message_id and boolean reconciliation", len(cliErr.Meta), cliErr.Meta) + } + if cliErr.Meta["message_id"] != int64(12345) { + t.Errorf("metadata = %#v, want only the known message ID and checks", cliErr.Meta) + } + + encodedChecks, marshalErr := json.Marshal(cliErr.Meta["reconciliation"]) + if marshalErr != nil { + t.Fatalf("marshal reconciliation checks: %v", marshalErr) + } + var checks map[string]any + if unmarshalErr := json.Unmarshal(encodedChecks, &checks); unmarshalErr != nil { + t.Fatalf("decode reconciliation checks: %v", unmarshalErr) + } + if len(checks) != len(wantChecks) { + t.Errorf("reconciliation = %s, want exactly %d boolean checks", encodedChecks, len(wantChecks)) + } + for name, want := range wantChecks { + got, exists := checks[name] + if !exists { + t.Errorf("reconciliation missing %q: %s", name, encodedChecks) + continue + } + gotBool, boolean := got.(bool) + if !boolean { + t.Errorf("reconciliation[%q] = %T, want bool", name, got) + continue + } + if gotBool != want { + t.Errorf("reconciliation[%q] = %v, want %v", name, gotBool, want) + } + } + for name, value := range checks { + if _, boolean := value.(bool); !boolean { + t.Errorf("reconciliation[%q] = %T, want bounded boolean-only metadata", name, value) + } + } + + encodedMeta, marshalErr := json.Marshal(cliErr.Meta) + if marshalErr != nil { + t.Fatalf("marshal ambiguity metadata: %v", marshalErr) + } + if len(encodedMeta) > 1024 { + t.Errorf("ambiguity metadata is %d bytes, want a bounded diagnostic", len(encodedMeta)) + } + for _, privateValue := range []string{"alice@example.com", "bob@example.com", "carol@example.org", "Inovo Customer Update", "

Body.

"} { + if strings.Contains(string(encodedMeta), privateValue) { + t.Errorf("ambiguity metadata leaked message data %q: %s", privateValue, encodedMeta) + } + } + assertVerifiableRequestLedger(t, state) +} + +func TestComposeVerifiableCreatesAKnownDraftThenSendsAndReadsOnlyThatID(t *testing.T) { + state := &verifiableComposeState{} + server := verifiableComposeServer(t, state) + + stdout, _, err := runVerifiableCompose(t, server) + if err != nil { + t.Fatalf("compose --verifiable: %v", err) + } + + envelope := composeJSON(t, stdout) + if !envelope.OK || !envelope.Data.Sent { + t.Fatalf("envelope = %+v, want a verified send", envelope) + } + if envelope.Data.MessageID != 12345 || envelope.Data.TopicID != 7742 || envelope.Data.AppURL != "https://app.hey.com/topics/7742" { + t.Errorf("handle = message %d topic %d url %q", envelope.Data.MessageID, envelope.Data.TopicID, envelope.Data.AppURL) + } + if envelope.Data.Verification.Status != verificationVerified { + t.Errorf("verification = %+v, want verified", envelope.Data.Verification) + } + if !envelope.Data.Verification.Recipients.BCCDisclosed { + t.Error("the exact BCC readback must be disclosed") + } + assertVerifiableRequestLedger(t, state) + + draftEntry, _ := state.draftBody["entry"].(map[string]any) + if draftEntry["status"] != "drafted" { + t.Errorf("draft entry status = %v, want drafted", draftEntry["status"]) + } + sendEntry, _ := state.sendBody["entry"].(map[string]any) + if _, drafted := sendEntry["status"]; drafted { + t.Errorf("send entry status = %v, want omitted to deliver", sendEntry["status"]) + } + if state.draftBody["acting_sender_id"] != float64(42) || state.sendBody["acting_sender_id"] != float64(42) { + t.Errorf("acting sender changed: draft=%v send=%v", state.draftBody["acting_sender_id"], state.sendBody["acting_sender_id"]) + } +} + +func TestComposeVerifiableAcceptsAnExplicitlyEmptyExactBCC(t *testing.T) { + readback := exactVerifiableReadback() + readback.bcc = nil + state := &verifiableComposeState{readJSON: encodeVerifiableReadback(t, readback)} + server := verifiableComposeServer(t, state) + + stdout, _, err := runCLIRaw(t, server, "--json", "--account", "840304", "compose", "--verifiable", + "--to", "alice@example.com", "--cc", "bob@example.com", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("compose --verifiable with an explicitly empty BCC readback: %v", err) + } + verification := composeJSON(t, stdout).Data.Verification + if verification.Status != verificationVerified || !verification.Recipients.BCCDisclosed || len(verification.Recipients.BCC) != 0 { + t.Errorf("verification = %+v, want a verified, explicitly empty BCC", verification) + } + assertVerifiableRequestLedger(t, state) +} + +func TestComposeVerifiableReconcilesAnAmbiguousSendByItsKnownID(t *testing.T) { + state := &verifiableComposeState{sendStatus: http.StatusInternalServerError} + server := verifiableComposeServer(t, state) + + stdout, _, err := runVerifiableCompose(t, server) + if err != nil { + t.Fatalf("the exact known-ID readback should reconcile the ambiguous send: %v", err) + } + envelope := composeJSON(t, stdout) + if envelope.Data.MessageID != 12345 || envelope.Data.Verification.Status != verificationVerified { + t.Errorf("result = %+v, want the verified draft ID 12345", envelope.Data) + } + assertVerifiableRequestLedger(t, state) +} + +func TestComposeVerifiableLeavesAnUnreadableKnownIDUnknownWithoutRetrying(t *testing.T) { + state := &verifiableComposeState{readStatus: http.StatusNotFound, readJSON: `{"message":"not found"}`} + server := verifiableComposeServer(t, state) + + stdout, _, err := runVerifiableCompose(t, server) + assertUnknownVerifiableCompose(t, stdout, err, state, allVerifiableChecks(false)) +} + +func TestComposeVerifiableRequiresEveryExactReadbackIdentityCheck(t *testing.T) { + tests := []struct { + name string + change func(*verifiableReadback) + wantChecks map[string]bool + }{ + { + name: "wrong message id", + change: func(readback *verifiableReadback) { + readback.messageID = 99999 + }, + wantChecks: verifiableChecksWith(map[string]bool{"message_id": false}), + }, + { + name: "no delivered topic", + change: func(readback *verifiableReadback) { + readback.url = "https://app.hey.com/messages/12345/edit" + }, + wantChecks: verifiableChecksWith(map[string]bool{"delivery_topic": false}), + }, + { + name: "wrong sender", + change: func(readback *verifiableReadback) { + readback.senderID = 99 + }, + wantChecks: verifiableChecksWith(map[string]bool{"sender": false}), + }, + { + name: "wrong subject", + change: func(readback *verifiableReadback) { + readback.subject = "A different subject" + }, + wantChecks: verifiableChecksWith(map[string]bool{"verification_status": false, "subject": false}), + }, + { + name: "wrong To", + change: func(readback *verifiableReadback) { + readback.to = []string{"mallory@example.com"} + }, + wantChecks: verifiableChecksWith(map[string]bool{"verification_status": false, "to": false}), + }, + { + name: "wrong CC", + change: func(readback *verifiableReadback) { + readback.cc = []string{"dana@example.org"} + }, + wantChecks: verifiableChecksWith(map[string]bool{"verification_status": false, "cc": false}), + }, + { + name: "undisclosed BCC", + change: func(readback *verifiableReadback) { + readback.bccDisclosed = false + }, + wantChecks: verifiableChecksWith(map[string]bool{"bcc_disclosed": false, "bcc": false}), + }, + { + name: "changed BCC", + change: func(readback *verifiableReadback) { + readback.bcc = []string{"dana@example.org"} + }, + wantChecks: verifiableChecksWith(map[string]bool{"verification_status": false, "bcc": false}), + }, + { + name: "changed body", + change: func(readback *verifiableReadback) { + readback.content = "

Something else.

" + }, + wantChecks: verifiableChecksWith(map[string]bool{"verification_status": false, "body": false}), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + readback := exactVerifiableReadback() + test.change(&readback) + state := &verifiableComposeState{readJSON: encodeVerifiableReadback(t, readback)} + server := verifiableComposeServer(t, state) + + stdout, _, err := runVerifiableCompose(t, server) + assertUnknownVerifiableCompose(t, stdout, err, state, test.wantChecks) + }) + } +} + +func TestComposeVerifiableRejectsDuplicateRecipientSubstitution(t *testing.T) { + tests := []struct { + name string + args []string + change func(*verifiableReadback) + failedCheck string + }{ + { + name: "To", + args: []string{"--to", "alice@example.com,dana@example.com", "--cc", "bob@example.com", "--bcc", "carol@example.org"}, + change: func(r *verifiableReadback) { r.to = []string{"alice@example.com", "alice@example.com"} }, + failedCheck: "to", + }, + { + name: "CC", + args: []string{"--to", "alice@example.com", "--cc", "bob@example.com,dana@example.com", "--bcc", "carol@example.org"}, + change: func(r *verifiableReadback) { r.cc = []string{"bob@example.com", "bob@example.com"} }, + failedCheck: "cc", + }, + { + name: "BCC", + args: []string{"--to", "alice@example.com", "--cc", "bob@example.com", "--bcc", "carol@example.org,dana@example.com"}, + change: func(r *verifiableReadback) { r.bcc = []string{"carol@example.org", "carol@example.org"} }, + failedCheck: "bcc", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + readback := exactVerifiableReadback() + test.change(&readback) + state := &verifiableComposeState{readJSON: encodeVerifiableReadback(t, readback)} + server := verifiableComposeServer(t, state) + args := []string{"--json", "--account", "840304", "compose", "--verifiable"} + args = append(args, test.args...) + args = append(args, "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + stdout, _, err := runCLIRaw(t, server, args...) + want := map[string]bool{test.failedCheck: false} + if test.name != "BCC" { + want["verification_status"] = false + } + assertUnknownVerifiableCompose(t, stdout, err, state, verifiableChecksWith(want)) + }) + } +} + +func TestComposeVerifiableRejectsUnverifiableInputsBeforeAnyWrite(t *testing.T) { + tests := []struct { + name string + args []string + }{ + {name: "attachment", args: []string{"-m", "Body.", "--attach", "report.pdf"}}, + {name: "raw HTML", args: []string{"--message-html", "

Body.

"}}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + state := &verifiableComposeState{} + server := verifiableComposeServer(t, state) + args := []string{"--json", "--account", "840304", "compose", "--verifiable", + "--to", "alice@example.com", "--subject", "Test"} + args = append(args, test.args...) + _, _, err := runCLIRaw(t, server, args...) + if err == nil || !strings.Contains(err.Error(), "--verifiable") { + t.Fatalf("error = %v, want a --verifiable incompatibility", err) + } + if got := strings.Join(state.requests, ", "); got != "GET /identity.json" { + t.Fatalf("requests = %v, want only read-only account identity before failure", state.requests) + } + }) + } +} + +func TestComposeExplicitFalseFlagsDoNotConflict(t *testing.T) { + state := &verifiableComposeState{} + server := verifiableComposeServer(t, state) + _, _, err := runCLIRaw(t, server, "--json", "--account", "840304", "compose", + "--verifiable=false", "--draft=false", "--to", "alice@example.com", + "--cc", "bob@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err != nil { + t.Fatalf("explicitly false flags must leave direct compose enabled: %v", err) + } +} + +func TestComposeVerifiableRejectsHTMLMarkupPassedAsMarkdown(t *testing.T) { + for _, message := range []string{ + `

Body.

`, + ``, + } { + state := &verifiableComposeState{} + server := verifiableComposeServer(t, state) + _, _, err := runCLIRaw(t, server, "--json", "--account", "840304", "compose", "--verifiable", + "--to", "alice@example.com", "--subject", "Test", "-m", message) + if err == nil { + t.Fatal("verifiable compose accepted HTML markup through the Markdown input") + } + if state.draftWrites != 0 || state.sendWrites != 0 { + t.Fatalf("draft/send writes = %d/%d, want 0/0", state.draftWrites, state.sendWrites) + } + } +} + +func TestComposeVerifiableRejectsTopicOutsideCanonicalPath(t *testing.T) { + for _, rawURL := range []string{ + "https://app.hey.com/messages/12345/edit?return_to=/topics/7742", + "https://app.hey.com/messages/12345/topics/7742", + "https://app.hey.com/topics/7742/", + "https://app.hey.com/topics/+7742", + "topics/7742", + } { + readback := exactVerifiableReadback() + readback.url = rawURL + state := &verifiableComposeState{readJSON: encodeVerifiableReadback(t, readback)} + server := verifiableComposeServer(t, state) + stdout, _, err := runVerifiableCompose(t, server) + assertUnknownVerifiableCompose(t, stdout, err, state, + verifiableChecksWith(map[string]bool{"delivery_topic": false})) + } +} + +func TestComposeVerifiableDoesNotReplaceExplicitSenderIDWithCreator(t *testing.T) { + state := &verifiableComposeState{readJSON: `{ + "id":12345,"url":"https://app.hey.com/topics/7742", + "subject":"Inovo Customer Update — Week 12","content":"

Body.

", + "sender":{"id":99}, + "creator":{"id":42,"name":"Nova Desk","email_address":"nova@example.com"}, + "addressed":{ + "directly":[{"id":1,"email_address":"alice@example.com"}], + "copied":[{"id":2,"email_address":"bob@example.com"}], + "blindcopied":[{"id":3,"email_address":"carol@example.org"}] + } + }`} + server := verifiableComposeServer(t, state) + stdout, _, err := runVerifiableCompose(t, server) + assertUnknownVerifiableCompose(t, stdout, err, state, + verifiableChecksWith(map[string]bool{"sender": false})) +} + +func TestComposeVerifiableRequiresExplicitSenderInReadback(t *testing.T) { + state := &verifiableComposeState{readJSON: `{ + "id":12345,"url":"https://app.hey.com/topics/7742", + "subject":"Inovo Customer Update — Week 12","content":"

Body.

", + "creator":{"id":42,"name":"Nova Desk","email_address":"nova@example.com"}, + "addressed":{ + "directly":[{"id":1,"email_address":"alice@example.com"}], + "copied":[{"id":2,"email_address":"bob@example.com"}], + "blindcopied":[{"id":3,"email_address":"carol@example.org"}] + } + }`} + server := verifiableComposeServer(t, state) + stdout, _, err := runVerifiableCompose(t, server) + assertUnknownVerifiableCompose(t, stdout, err, state, + verifiableChecksWith(map[string]bool{"sender": false})) +} + +func TestComposeVerifiableRejectsRecipientContactWithoutAddress(t *testing.T) { + state := &verifiableComposeState{readJSON: `{ + "id":12345,"url":"https://app.hey.com/topics/7742", + "subject":"Inovo Customer Update — Week 12","content":"

Body.

", + "sender":{"id":42,"name":"Nova Desk","email_address":"nova@example.com"}, + "addressed":{ + "directly":[{"id":1,"email_address":"alice@example.com"},{"id":999,"name":"Undisclosed recipient"}], + "copied":[{"id":2,"email_address":"bob@example.com"}], + "blindcopied":[{"id":3,"email_address":"carol@example.org"}] + } + }`} + server := verifiableComposeServer(t, state) + _, _, err := runVerifiableCompose(t, server) + if err == nil { + t.Fatal("verifiable compose accepted a recipient contact with no address") + } + assertVerifiableRequestLedger(t, state) +} + +func TestComposeVerifiableRejectsNumericNonMessageDraftLocation(t *testing.T) { + puts := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/identity.json": + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"id":1,"accounts":[{"id":840304,"status":"active"}],"senders":[{"id":42,"account_id":840304,"default":true}]}`) + case r.Method == http.MethodPost && r.URL.Path == "/messages.json": + w.Header().Set("Location", "https://app.hey.com/topics/12345") + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodGet && r.URL.Path == "/messages/12345/edit.json": + http.NotFound(w, r) + case r.Method == http.MethodPut && r.URL.Path == "/messages/12345.json": + puts++ + w.WriteHeader(http.StatusOK) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + _, _, err := runVerifiableCompose(t, server) + if err == nil || puts != 0 { + t.Fatalf("numeric /topics Location was accepted as a draft message id: err=%v PUTs=%d", err, puts) + } +} diff --git a/internal/cmd/compose_verify.go b/internal/cmd/compose_verify.go index 6de267a5..75b21bac 100644 --- a/internal/cmd/compose_verify.go +++ b/internal/cmd/compose_verify.go @@ -152,7 +152,7 @@ func verifyComposedMessage(ctx context.Context, client *hey.Client, messageID in // when HEY names no separate sender. func senderOf(message *generated.Message) *threadContact { contact := message.Sender - if contact.EmailAddress == "" && contact.Name == "" { + if contact.Id == 0 && contact.EmailAddress == "" && contact.Name == "" { contact = message.Creator } if contact.EmailAddress == "" && contact.Name == "" && contact.Id == 0 { @@ -199,7 +199,8 @@ func bodyDigest(markdown string) string { // empty one mean different things, and only the caller knows which it is willing to // accept. func recipientsMatch(sent composeSent, got addressedEnvelope) bool { - return sameAddresses(sent.To, got.To) && + return !got.Truncated && + sameAddresses(sent.To, got.To) && sameAddresses(sent.CC, got.CC) && addressSubset(got.BCC, sent.BCC) } @@ -208,7 +209,18 @@ func sameAddresses(want, got []string) bool { if len(want) != len(got) { return false } - return addressSubset(got, want) + remaining := make(map[string]int, len(want)) + for _, address := range want { + remaining[normalizeAddress(address)]++ + } + for _, address := range got { + normalized := normalizeAddress(address) + if remaining[normalized] == 0 { + return false + } + remaining[normalized]-- + } + return true } func addressSubset(got, want []string) bool { diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 3c1206cf..95c0817a 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -118,7 +118,7 @@ func newRootCmd() *cobra.Command { authMgr = auth.NewManager(cfg.BaseURL, httpClient, configDir) initSDK(authMgr, cfg.BaseURL) - // The agent-local setup subcommands and skill commands never read + // Local introspection, agent-local setup subcommands and skill commands never read // credentials and never rewrite the global config, so they defer // migration — the installer runs them with HEY_NO_KEYRING=1, which // would otherwise move legacy tokens into plaintext. Every other @@ -245,7 +245,7 @@ func commandSkipsCredentialMigration(cmd *cobra.Command) bool { return false } switch parts[1] { - case "skill": + case "commands", "version", "skill": return true case "setup": return len(parts) > 2 // the wizard itself persists onboarded; subcommands touch only agent files diff --git a/internal/cmd/root_test.go b/internal/cmd/root_test.go index cdf6080c..1027563d 100644 --- a/internal/cmd/root_test.go +++ b/internal/cmd/root_test.go @@ -27,6 +27,19 @@ func stubRunTUI(t *testing.T) *int { return &calls } +func TestReadOnlyIntrospectionSkipsCredentialMigration(t *testing.T) { + root := newRootCmd() + for _, name := range []string{"version", "commands"} { + command, _, err := root.Find([]string{name}) + if err != nil { + t.Fatalf("find %s: %v", name, err) + } + if !commandSkipsCredentialMigration(command) { + t.Errorf("%s should not migrate credentials", name) + } + } +} + func stubAskToSignIn(t *testing.T, answer bool) *int { t.Helper() calls := 0 diff --git a/skills/hey/SKILL.md b/skills/hey/SKILL.md index e86dad42..e6b1d3ef 100644 --- a/skills/hey/SKILL.md +++ b/skills/hey/SKILL.md @@ -199,6 +199,7 @@ notice on stderr. Both need list data, so they work on `hey box list`, `hey box | Forward email | `hey forward --to alice@example.com -m "For your review"` | | Compose email | `hey compose --to alice@example.com --subject "Lunch plans" -m "Are you free Friday?"` | | Compose with CC/BCC | `hey compose --to alice@example.com --cc bob@example.com --bcc carol@example.org --subject "Kitchen remodel timeline"` | +| Compose with exact known-ID reconciliation | `hey compose --to alice@example.com --subject "Customer update" -m "The migration is complete." --verifiable` (pre-send draft readback; no drafts, replies, attachments, or HTML markup in the Markdown input) | | List drafts | `hey draft list --json` (`--all`/`--page` follow the cursor) | | Draft an email for human review | `hey compose --to alice@example.com --subject "Lunch plans" -m "Free Friday?" --draft` | | Draft a reply for human review | `hey reply -m "Drafting this." --draft` | @@ -305,6 +306,7 @@ Want to send email? │ └── Add a note? → add -m "note" ├── Compose new? → hey compose --to --subject "Subject" │ ├── With body? → hey compose --to --subject "Subject" -m "Body" +│ ├── Need exact known-ID readback? → add --verifiable (draft readback, one send, no search/retry/files/HTML markup) │ ├── With files? → add --attach ./report.pdf (repeatable; body is optional) │ ├── With CC? → add --cc │ └── With BCC? → add --bcc @@ -476,6 +478,7 @@ hey compose --to alice@example.com --subject "Q3 revenue report" -m "The numbers hey compose --to alice@example.com --cc bob@example.com --bcc carol@example.org --subject "Kitchen remodel timeline" -m "Cabinets land the week of the 14th." hey compose --thread-id 12345 -m "Confirmed — see you then." # Reply into an existing thread (no subject: it carries the thread's) hey compose --to alice@example.com --subject "Sprint recap" -m "We **shipped** the pagination fix." +hey compose --to alice@example.com --subject "Customer update" -m "The migration is complete." --verifiable # Known-ID send with exact readback hey compose --to alice@example.com --subject "Newsletter draft" --message-html "

March

What we shipped.

" ``` From 3397183148acc25a92c31d5e46196260eb23da62 Mon Sep 17 00:00:00 2001 From: jonata97 Date: Fri, 4 Sep 2026 16:52:46 -0300 Subject: [PATCH 5/5] fix(compose): prevent auth refresh from replaying delivery --- internal/cmd/compose_verifiable_test.go | 54 +++++++++++++++++++++++-- internal/cmd/root.go | 7 +++- internal/cmd/sdk.go | 28 ++++++++++++- 3 files changed, 84 insertions(+), 5 deletions(-) diff --git a/internal/cmd/compose_verifiable_test.go b/internal/cmd/compose_verifiable_test.go index f4dabd9b..97f43bd7 100644 --- a/internal/cmd/compose_verifiable_test.go +++ b/internal/cmd/compose_verifiable_test.go @@ -6,10 +6,13 @@ import ( "fmt" "net/http" "net/http/httptest" + "path/filepath" "strings" "testing" + "time" "github.com/basecamp/hey-cli/internal/apierr" + "github.com/basecamp/hey-cli/internal/auth" ) type verifiableComposeState struct { @@ -20,9 +23,11 @@ type verifiableComposeState struct { requests []string accountScopes map[string][]string - sendStatus int - readStatus int - readJSON string + sendStatus int + readStatus int + readJSON string + unauthorizedFirstSend bool + refreshRequests int draftBody map[string]any sendBody map[string]any @@ -137,11 +142,19 @@ func verifiableComposeServer(t *testing.T, state *verifiableComposeState) *httpt case r.Method == http.MethodPut && r.URL.Path == "/messages/12345.json": state.sendWrites++ _ = json.NewDecoder(r.Body).Decode(&state.sendBody) + if state.unauthorizedFirstSend && state.sendWrites == 1 { + w.WriteHeader(http.StatusUnauthorized) + return + } status := state.sendStatus if status == 0 { status = http.StatusOK } w.WriteHeader(status) + case r.Method == http.MethodPost && r.URL.Path == "/oauth/tokens": + state.refreshRequests++ + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"new-access","refresh_token":"new-refresh","expires_in":3600}`) case r.Method == http.MethodGet && r.URL.Path == "/messages/12345.json": state.messageReads++ status := state.readStatus @@ -321,6 +334,41 @@ func TestComposeVerifiableCreatesAKnownDraftThenSendsAndReadsOnlyThatID(t *testi } } +func TestComposeVerifiableDoesNotRefreshAndResendARejectedDelivery(t *testing.T) { + state := &verifiableComposeState{unauthorizedFirstSend: true} + server := verifiableComposeServer(t, state) + configHome := t.TempDir() + t.Setenv("HEY_NO_KEYRING", "1") + manager := auth.NewManager(server.URL, server.Client(), filepath.Join(configHome, "hey-cli")) + if err := manager.GetStore().Save(manager.CredentialKey(), &auth.Credentials{ + AccessToken: "old-access", RefreshToken: "old-refresh", ExpiresAt: time.Now().Add(time.Hour).Unix(), + }); err != nil { + t.Fatalf("seed credentials: %v", err) + } + + stdout, _, err := runAuthCommand(t, configHome, server.URL, "", true, + "--account", "840304", "compose", "--verifiable", + "--to", "alice@example.com", "--cc", "bob@example.com", "--bcc", "carol@example.org", + "--subject", "Inovo Customer Update — Week 12", "-m", "Body.") + if err == nil { + t.Fatal("compose --verifiable accepted a rejected delivery") + } + if stdout != "" { + t.Errorf("stdout = %q, want no success envelope", stdout) + } + var cliErr *apierr.Error + if !errors.As(err, &cliErr) || cliErr.Code == apierr.CodeAmbiguous { + t.Fatalf("error = %v, want a terminal non-success result", err) + } + if state.sendWrites != 1 || state.refreshRequests != 0 || state.messageReads != 0 { + t.Fatalf("send writes/refreshes/readbacks = %d/%d/%d, want 1/0/0", + state.sendWrites, state.refreshRequests, state.messageReads) + } + if got := strings.Join(state.requests, ", "); strings.Contains(got, "search") { + t.Fatalf("requests = %s, want no search or fallback", got) + } +} + func TestComposeVerifiableAcceptsAnExplicitlyEmptyExactBCC(t *testing.T) { readback := exactVerifiableReadback() readback.bcc = nil diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 95c0817a..450de8e8 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -116,7 +116,12 @@ func newRootCmd() *cobra.Command { configDir := config.ConfigDir() httpClient := &http.Client{Timeout: 30 * time.Second} authMgr = auth.NewManager(cfg.BaseURL, httpClient, configDir) - initSDK(authMgr, cfg.BaseURL) + verifiableCompose, _ := cmd.Flags().GetBool("verifiable") + if cmd.Name() == "compose" && verifiableCompose { + initSDKWithoutRefresh(authMgr, cfg.BaseURL) + } else { + initSDK(authMgr, cfg.BaseURL) + } // Local introspection, agent-local setup subcommands and skill commands never read // credentials and never rewrite the global config, so they defer diff --git a/internal/cmd/sdk.go b/internal/cmd/sdk.go index c064d2d4..9dd225a7 100644 --- a/internal/cmd/sdk.go +++ b/internal/cmd/sdk.go @@ -51,6 +51,18 @@ func (a *cliAuthStrategy) Refresh(ctx context.Context) error { return a.mgr.Refresh(ctx) } +// cliAuthOnlyStrategy deliberately does not implement hey.TokenRefresher. Verifiable +// compose uses it because replaying a delivery PUT after a 401 would violate its one-send +// contract; AuthenticateRequest may still refresh an already-expired token before the first +// request, but the SDK cannot refresh-and-retry a request the server has answered. +type cliAuthOnlyStrategy struct { + mgr *auth.Manager +} + +func (a *cliAuthOnlyStrategy) Authenticate(ctx context.Context, req *http.Request) error { + return a.mgr.AuthenticateRequest(ctx, req) +} + // statsHooks implements hey.Hooks for --stats tracking. type statsHooks struct { requestCount atomic.Int64 @@ -77,6 +89,16 @@ var sdkStats *statsHooks // initSDK creates the SDK client, bridging the CLI's auth and config. func initSDK(authMgr *auth.Manager, baseURL string) { + initSDKWithRefresh(authMgr, baseURL, true) +} + +// initSDKWithoutRefresh builds the command-scoped client for compose --verifiable. Other +// commands retain the SDK's normal one-refresh behavior through initSDK. +func initSDKWithoutRefresh(authMgr *auth.Manager, baseURL string) { + initSDKWithRefresh(authMgr, baseURL, false) +} + +func initSDKWithRefresh(authMgr *auth.Manager, baseURL string, refreshOnUnauthorized bool) { sdkCfg := &hey.Config{ BaseURL: baseURL, } @@ -90,7 +112,11 @@ func initSDK(authMgr *auth.Manager, baseURL string) { } var opts []hey.ClientOption - opts = append(opts, hey.WithAuthStrategy(&cliAuthStrategy{mgr: authMgr})) + if refreshOnUnauthorized { + opts = append(opts, hey.WithAuthStrategy(&cliAuthStrategy{mgr: authMgr})) + } else { + opts = append(opts, hey.WithAuthStrategy(&cliAuthOnlyStrategy{mgr: authMgr})) + } opts = append(opts, hey.WithUserAgent(version.UserAgent()+" "+hey.DefaultUserAgent)) if verboseFlag > 0 {