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,6 +264,12 @@ 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` 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. + `hey shareHi 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/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..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 "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,11 +108,23 @@ 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) } 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 +147,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 classifySendFailure(sendErr) } } else { to := parseAddresses(c.to) @@ -145,12 +182,50 @@ 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} + 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 { + return classifySendFailure(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), + "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) + result := composeResultFor(handle, verification, readback) + return writeComposedMessage(cmd, result, 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{ + 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. @@ -180,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_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_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..a259767a --- /dev/null +++ b/internal/cmd/compose_send.go @@ -0,0 +1,167 @@ +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 +// 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, notDispatched(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, notDispatched(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) +} + +// --- 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" + } +} 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..97f43bd7 --- /dev/null +++ b/internal/cmd/compose_verifiable_test.go @@ -0,0 +1,695 @@ +package cmd + +import ( + "encoding/json" + "errors" + "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 { + draftWrites int + draftReads int + sendWrites int + messageReads int + requests []string + accountScopes map[string][]string + + sendStatus int + readStatus int + readJSON string + unauthorizedFirstSend bool + refreshRequests int + + 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) + 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 + 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 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 + 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.
`, + `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 new file mode 100644 index 00000000..75b21bac --- /dev/null +++ b/internal/cmd/compose_verify.go @@ -0,0 +1,279 @@ +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. 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"` +} + +// 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.Id == 0 && 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 +// 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 !got.Truncated && + 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 + } + 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 { + 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..06b31233 --- /dev/null +++ b/internal/cmd/compose_verify_test.go @@ -0,0 +1,639 @@ +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") + } +} + +// 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/root.go b/internal/cmd/root.go index 3c1206cf..450de8e8 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -116,9 +116,14 @@ 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) + } - // 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 +250,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/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 { diff --git a/internal/cmd/thread_envelope_test.go b/internal/cmd/thread_envelope_test.go new file mode 100644 index 00000000..82bf40b0 --- /dev/null +++ b/internal/cmd/thread_envelope_test.go @@ -0,0 +1,291 @@ +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,
\nHi 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") + } +} + +// 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/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/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 forwardWhat we shipped.
" ``` diff --git a/tests/smoke/compose_test.go b/tests/smoke/compose_test.go index 8e9229db..95ae5cc8 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,83 @@ 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) + } + // 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) + } + // 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) } }