From fae263ef6778826e3ee945af05b1d69bcbbe6030 Mon Sep 17 00:00:00 2001 From: Ellis Givens Date: Wed, 19 Aug 2026 13:02:51 -0500 Subject: [PATCH 1/3] Improve reply preview and delivery verification --- .surface | 2 + API-COVERAGE.md | 5 +- README.md | 7 +- internal/cmd/attachments_test.go | 85 +++++++-- internal/cmd/compose_test.go | 2 +- internal/cmd/reply.go | 284 ++++++++++++++++++++++++++++-- internal/cmd/reply_test.go | 280 +++++++++++++++++++++++++++++ internal/cmd/thread_reply.go | 42 +++-- internal/cmd/thread_reply_test.go | 93 ++++++---- internal/cmd/topic.go | 4 +- internal/htmlutil/reply.go | 126 +++++++++++++ internal/htmlutil/reply_test.go | 80 +++++++++ skills/hey/SKILL.md | 16 +- tests/smoke/compose_test.go | 15 +- 14 files changed, 949 insertions(+), 92 deletions(-) create mode 100644 internal/cmd/reply_test.go create mode 100644 internal/htmlutil/reply.go create mode 100644 internal/htmlutil/reply_test.go diff --git a/.surface b/.surface index 984150af..daddc3d5 100644 --- a/.surface +++ b/.surface @@ -96,7 +96,9 @@ hey recordings --limit hey recordings --starts-on hey reply hey reply --attach +hey reply --expect-entry hey reply --message +hey reply --preview hey search hey search --all hey search --any diff --git a/API-COVERAGE.md b/API-COVERAGE.md index 8ebc25dc..02e3572f 100644 --- a/API-COVERAGE.md +++ b/API-COVERAGE.md @@ -34,8 +34,9 @@ The remaining HTML-reading gaps use the SDK's authenticated HTML helper and are | signed Active Storage upload URL | PUT | SDK `Attachments().Upload` | `hey compose --attach`, `hey reply --attach` | covered | | signed Active Storage blob URL | GET | SDK `DownloadBlob` | `hey attachments save ` | covered | | `/messages.json` | POST | SDK `Messages().Create` | `hey compose`, `hey forward ` | covered | -| `/entries/{id}/replies` | POST | SDK `Entries().CreateReply` | `hey reply ` | covered | -| `/topics/{id}.json` | GET | SDK `Topics().Get` | `hey forward ` | covered | +| `/entries/{id}/replies/new` | GET (HTML) | SDK `GetHTML` | `hey reply `, `hey compose --thread-id` | gap: resolves the live reply envelope | +| `/entries/{id}/replies.json` | POST | SDK `Entries().CreateReply` | `hey reply ` | covered | +| `/topics/{id}.json` | GET | SDK `Topics().Get` | `hey forward `, `hey reply ` | covered | | `/entries/{id}/forwards/new.json` | GET | SDK `Entries().NewForward` | `hey forward ` | covered | | `/postings/moves.json` | POST | SDK `Postings().Move` | `hey move --to `, TUI `m` | covered | | `/postings/trash.json` | POST | SDK `Postings().MoveToTrash` | `hey trash `, TUI `t` | covered | diff --git a/README.md b/README.md index 4567d0db..8b281eff 100644 --- a/README.md +++ b/README.md @@ -87,8 +87,9 @@ hey contacts note delete 12345 hey threads 123 # read a full email thread hey attachments 123 # list files attached to the thread hey attachments save 456:1 # save a file using its attachment ID -hey reply 123 -m "Thanks!" # reply to a thread (or omit -m to open $EDITOR) -hey reply 123 -m "Attached." --attach ./diagram.png +hey reply 123 -m "Thanks!" --preview # preview recipients, subject, body, and files +hey reply 123 -m "Thanks!" --expect-entry 456 # send only to the previewed entry +hey reply 123 -m "Attached." --attach ./diagram.png --preview hey forward 123 --to alice@example.com -m "For your review" # forward the latest message hey compose --to user@example.com --subject "Hello" # compose a new message hey compose --to user@example.com --subject "Report" -m "Attached." --attach ./report.pdf @@ -106,7 +107,7 @@ Search accepts free text plus `--required`, `--any`, `--none`, `--exact`, `--fro Contact updates preserve omitted name, email, and alias fields. Supplying `--alias` replaces the complete alias list; `--alias=` clears it. Contact notes accept positional content, `--note`, stdin, or `$EDITOR`. HEY hides contacts rather than permanently deleting them; hidden contacts leave lists, autocomplete, and search, and can be shown again by ID. -`--attach` is repeatable on `hey compose` and `hey reply`, and attachment-only messages are supported. The CLI validates and uploads every file before sending the email. `hey attachments ` returns stable message-and-position IDs such as `456:1`; pass an ID to `hey attachments save`. Saving uses the original filename by default, accepts `--output` for a file or directory, and preserves existing files unless `--force` is set. +`--attach` is repeatable on `hey compose` and `hey reply`, and attachment-only messages are supported. `hey reply --preview` inspects attachment files locally but does not upload or send anything. The preview returns an `entry_id`; pass it to `--expect-entry` when sending. If a newer entry arrived after the preview, the send stops and asks you to preview again. Sending validates and uploads every file before delivering the email. `hey attachments ` returns stable message-and-position IDs such as `456:1`; pass an ID to `hey attachments save`. Saving uses the original filename by default, accepts `--output` for a file or directory, and preserves existing files unless `--force` is set. Organization actions take the `id` values returned by `hey box --json` or `hey search --json`. Move destinations are Imbox, The Feed, Set Aside, Reply Later, or Paper Trail. Bubble Up requires a scheduled date and is not available through `hey move`. Trashing a shared thread removes your access instead of deleting it for everyone. Ignored threads remain in their box and can be restored with `hey stop-ignoring`. diff --git a/internal/cmd/attachments_test.go b/internal/cmd/attachments_test.go index a93ccff2..10b7a048 100644 --- a/internal/cmd/attachments_test.go +++ b/internal/cmd/attachments_test.go @@ -17,13 +17,15 @@ import ( ) type attachmentServerState struct { - mu sync.Mutex - directUploads int - storageUploads int - sentContents []string - events []string - blobStatus int - nilMessage bool + mu sync.Mutex + directUploads int + storageUploads int + sentContents []string + events []string + blobStatus int + nilMessage bool + advanceOnUpload bool + threadAdvanced bool } func attachmentServer(t *testing.T) (*httptest.Server, *attachmentServerState) { @@ -77,16 +79,47 @@ func attachmentServer(t *testing.T) (*httptest.Server, *attachmentServerState) { state.mu.Lock() state.storageUploads++ state.events = append(state.events, "upload") + if state.advanceOnUpload { + state.threadAdvanced = true + } state.mu.Unlock() w.WriteHeader(http.StatusNoContent) case r.Method == http.MethodGet && r.URL.Path == "/identity.json": _, _ = w.Write([]byte(`{"id":1,"senders":[{"id":42,"default":true}]}`)) - case r.Method == http.MethodGet && r.URL.Path == "/topics/7": - w.Header().Set("Content-Type", "text/html") - _, _ = w.Write([]byte(topicWithRecipients)) - case r.Method == http.MethodGet && r.URL.Path == "/topics/7/entries": + case r.Method == http.MethodGet && (r.URL.Path == "/topics/7" || r.URL.Path == "/topics/7.json"): + state.mu.Lock() + sentOrAdvanced := len(state.sentContents) > 0 || state.threadAdvanced + state.mu.Unlock() + latestEntryID := 12 + if sentOrAdvanced { + latestEntryID = 13 + } + _, _ = fmt.Fprintf(w, `{"id":7,"name":"Project update","latest_entry":{"id":%d}}`, latestEntryID) + case r.Method == http.MethodGet && r.URL.Path == "/topics/7/entries.json": + state.mu.Lock() + sent := len(state.sentContents) > 0 + state.mu.Unlock() + if sent && r.URL.Query().Get("page") == "1" { + _, _ = w.Write([]byte(`[{"id":13,"kind":"message","creator":{"id":42}}]`)) + } else { + _, _ = w.Write([]byte(`[]`)) + } + case r.Method == http.MethodGet && r.URL.Path == "/messages/13.json": + state.mu.Lock() + content := "" + if len(state.sentContents) > 0 { + content = state.sentContents[len(state.sentContents)-1] + } + state.mu.Unlock() + _ = json.NewEncoder(w).Encode(map[string]any{ + "id": 13, + "content": content, + "creator": map[string]any{"id": 42}, + "sender": map[string]any{"id": 42}, + }) + case r.Method == http.MethodGet && r.URL.Path == "/entries/12/replies/new": w.Header().Set("Content-Type", "text/html") - _, _ = w.Write([]byte(topicEntries)) + _, _ = w.Write([]byte(replyForm)) case r.Method == http.MethodPost && r.URL.Path == "/entries/12/replies.json": var body struct { Message struct { @@ -320,7 +353,7 @@ func TestReplyUploadsAttachmentsBeforeSending(t *testing.T) { if err := os.WriteFile(path, []byte("report contents"), 0o600); err != nil { t.Fatal(err) } - if _, err := runAttachmentCommand(t, server, "reply", "7", "-m", "Attached.", "--attach", path); err != nil { + if _, err := runAttachmentCommand(t, server, "reply", "7", "-m", "Attached.", "--attach", path, "--expect-entry", "12"); err != nil { t.Fatal(err) } @@ -331,13 +364,35 @@ func TestReplyUploadsAttachmentsBeforeSending(t *testing.T) { } } +func TestReplyRechecksPreviewedEntryAfterAttachmentUpload(t *testing.T) { + server, state := attachmentServer(t) + state.mu.Lock() + state.advanceOnUpload = true + state.mu.Unlock() + path := filepath.Join(t.TempDir(), "quarterly-report.pdf") + if err := os.WriteFile(path, []byte("report contents"), 0o600); err != nil { + t.Fatal(err) + } + + _, err := runAttachmentCommand(t, server, "reply", "7", "-m", "Attached.", "--attach", path, "--expect-entry", "12") + if err == nil || !strings.Contains(err.Error(), "thread changed after preview") { + t.Fatalf("error = %v, want stale preview rejection", err) + } + + state.mu.Lock() + defer state.mu.Unlock() + if len(state.sentContents) != 0 || strings.Contains(strings.Join(state.events, ","), "send") { + t.Errorf("stale reply was sent: %+v", state) + } +} + func TestReplySupportsAttachmentOnlyMessages(t *testing.T) { server, state := attachmentServer(t) path := filepath.Join(t.TempDir(), "quarterly-report.pdf") if err := os.WriteFile(path, []byte("report contents"), 0o600); err != nil { t.Fatal(err) } - if _, err := runAttachmentCommand(t, server, "reply", "7", "--attach", path); err != nil { + if _, err := runAttachmentCommand(t, server, "reply", "7", "--attach", path, "--expect-entry", "12"); err != nil { t.Fatal(err) } @@ -355,7 +410,7 @@ func TestReplyReadsPipedBodyWithAttachments(t *testing.T) { t.Fatal(err) } if _, err := runAttachmentCommandWithStdin(t, server, "Piped reply body\n", - "reply", "7", "--attach", path, + "reply", "7", "--attach", path, "--expect-entry", "12", ); err != nil { t.Fatal(err) } diff --git a/internal/cmd/compose_test.go b/internal/cmd/compose_test.go index cb66d01b..6990b082 100644 --- a/internal/cmd/compose_test.go +++ b/internal/cmd/compose_test.go @@ -66,7 +66,7 @@ func TestParseAddresses(t *testing.T) { // A reply carries the thread's subject with it, so --subject is only wanted when // starting a new thread. Requiring it either way made people pass one that HEY ignores. func TestComposeSubjectRequiredOnlyForANewMessage(t *testing.T) { - server, sent := threadReplyServer(t, topicWithRecipients, topicEntries) + server, sent := threadReplyServer(t, replyForm) err := runCLI(t, server, "compose", "-m", "body") var cliErr *apierr.Error diff --git a/internal/cmd/reply.go b/internal/cmd/reply.go index e03bc29e..cc7928a6 100644 --- a/internal/cmd/reply.go +++ b/internal/cmd/reply.go @@ -1,19 +1,28 @@ package cmd import ( + "context" "fmt" "strconv" + "strings" + "time" + "github.com/basecamp/hey-sdk/go/pkg/generated" "github.com/spf13/cobra" "github.com/basecamp/hey-cli/internal/editor" + "github.com/basecamp/hey-cli/internal/htmlutil" "github.com/basecamp/hey-cli/internal/output" ) +const maxReplyVerificationPages = 100 + type replyCommand struct { - cmd *cobra.Command - message string - attachments []string + cmd *cobra.Command + message string + attachments []string + preview bool + expectedEntryID int64 } func newReplyCommand() *replyCommand { @@ -22,17 +31,20 @@ func newReplyCommand() *replyCommand { Use: "reply ", Short: "Reply to a thread", Annotations: map[string]string{ - "agent_notes": "Replies to the latest entry in a thread. Accepts message via -m, stdin, or $EDITOR, plus repeatable --attach files; an attachment can be sent without body text.", + "agent_notes": "Run with --preview first, then pass its entry_id to --expect-entry when sending. The send stops if the thread changed after preview.", }, - Example: ` hey reply 12345 -m "Thanks!" - hey reply 12345 -m "Attached is the report." --attach ./report.pdf - echo "Detailed reply" | hey reply 12345`, + Example: ` hey reply 12345 -m "Thanks!" --preview + hey reply 12345 -m "Thanks!" --expect-entry 67890 + hey reply 12345 -m "Attached is the report." --attach ./report.pdf --preview + echo "Detailed reply" | hey reply 12345 --preview`, RunE: replyCommand.run, Args: usageExactOneArg(), } replyCommand.cmd.Flags().StringVarP(&replyCommand.message, "message", "m", "", "Reply message (or opens $EDITOR)") replyCommand.cmd.Flags().StringArrayVar(&replyCommand.attachments, "attach", nil, "File to attach (repeatable)") + replyCommand.cmd.Flags().BoolVar(&replyCommand.preview, "preview", false, "Preview the complete reply without sending") + replyCommand.cmd.Flags().Int64Var(&replyCommand.expectedEntryID, "expect-entry", 0, "Send only if this is still the thread's latest entry ID") return replyCommand } @@ -43,15 +55,19 @@ func (c *replyCommand) run(cmd *cobra.Command, args []string) error { } threadID, err := strconv.ParseInt(args[0], 10, 64) - if err != nil { + if err != nil || threadID <= 0 { return output.ErrUsage(fmt.Sprintf("invalid thread ID: %s", args[0])) } ctx := cmd.Context() - - target, err := resolveThreadReply(ctx, threadID) - if err != nil { - return err + if cmd.Flags().Changed("expect-entry") && c.expectedEntryID <= 0 { + return output.ErrUsage("--expect-entry must be a positive entry ID") + } + if !c.preview && c.expectedEntryID <= 0 { + return output.ErrUsageHint( + "sending a reply requires --expect-entry with the entry_id from a fresh preview", + fmt.Sprintf("Run: hey reply %d -m --preview --json", threadID), + ) } message := c.message @@ -73,13 +89,43 @@ func (c *replyCommand) run(cmd *cobra.Command, args []string) error { } } + if c.preview { + target, resolveErr := resolveThreadReply(ctx, threadID) + if resolveErr != nil { + return resolveErr + } + attachments, inspectErr := inspectReplyAttachments(c.attachments) + if inspectErr != nil { + return inspectErr + } + return writeReplyPreview(cmd, threadID, target, message, attachments) + } + + senderID, err := sdk.DefaultSenderID(ctx) + if err != nil { + return convertSDKError(err) + } + target, err := resolveExpectedThreadReply(ctx, threadID, c.expectedEntryID) + if err != nil { + return err + } message, err = attachFiles(ctx, message, c.attachments) if err != nil { return err } + if len(c.attachments) > 0 { + target, err = resolveExpectedThreadReply(ctx, threadID, c.expectedEntryID) + if err != nil { + return err + } + } if err = sdk.Entries().CreateReply(ctx, target.EntryID, message, target.Addressed.To, target.Addressed.CC, target.Addressed.BCC); err != nil { return convertSDKError(err) } + replyEntryID, err := verifyReplyCreated(ctx, threadID, target.EntryID, senderID, message, defaultReplyVerificationDelays()) + if err != nil { + return err + } summary := sentWithAttachmentsSummary("Reply sent", len(c.attachments)) if writer.IsStyled() { @@ -87,7 +133,10 @@ func (c *replyCommand) run(cmd *cobra.Command, args []string) error { return nil } - return writeOK(nil, + return writeOK(map[string]int64{ + "thread_id": threadID, + "entry_id": replyEntryID, + }, output.WithSummary(summary), output.WithBreadcrumbs(output.Breadcrumb{ Action: "view", @@ -96,3 +145,212 @@ func (c *replyCommand) run(cmd *cobra.Command, args []string) error { }), ) } + +func resolveExpectedThreadReply(ctx context.Context, threadID, expectedEntryID int64) (*threadReplyTarget, error) { + return resolveThreadReplyAtEntry(ctx, threadID, expectedEntryID) +} + +type replyPreviewAttachment struct { + Path string `json:"path"` + Filename string `json:"filename"` + ContentType string `json:"content_type"` + ByteSize int64 `json:"byte_size"` +} + +type replyPreview struct { + ThreadID int64 `json:"thread_id"` + EntryID int64 `json:"entry_id"` + From string `json:"from"` + To []string `json:"to"` + CC []string `json:"cc"` + BCC []string `json:"bcc"` + Subject string `json:"subject"` + Body string `json:"body"` + Attachments []replyPreviewAttachment `json:"attachments"` +} + +func inspectReplyAttachments(paths []string) ([]replyPreviewAttachment, error) { + if len(paths) == 0 { + return []replyPreviewAttachment{}, nil + } + prepared, err := prepareAttachments(paths) + if err != nil { + return nil, err + } + defer closePreparedAttachments(prepared) + + attachments := make([]replyPreviewAttachment, 0, len(prepared)) + for _, attachment := range prepared { + attachments = append(attachments, replyPreviewAttachment{ + Path: attachment.path, + Filename: attachment.filename, + ContentType: attachment.contentType, + ByteSize: attachment.byteSize, + }) + } + return attachments, nil +} + +func writeReplyPreview(cmd *cobra.Command, threadID int64, target *threadReplyTarget, message string, attachments []replyPreviewAttachment) error { + from, err := defaultReplySenderEmail(cmd.Context()) + if err != nil { + return err + } + subject := strings.TrimSpace(target.Subject) + if subject == "" { + return output.ErrAPI(0, "could not determine thread subject") + } + if !strings.HasPrefix(strings.ToLower(subject), "re:") { + subject = "Re: " + subject + } + + preview := replyPreview{ + ThreadID: threadID, + EntryID: target.EntryID, + From: from, + To: target.Addressed.To, + CC: target.Addressed.CC, + BCC: target.Addressed.BCC, + Subject: subject, + Body: message, + Attachments: attachments, + } + + if writer.IsStyled() { + w := cmd.OutOrStdout() + fmt.Fprintf(w, "Thread: %d\n", preview.ThreadID) + fmt.Fprintf(w, "Reply target: entry %d\n", preview.EntryID) + fmt.Fprintf(w, "From: %s\n", preview.From) + fmt.Fprintf(w, "To: %s\n", strings.Join(preview.To, ", ")) + fmt.Fprintf(w, "CC: %s\n", strings.Join(preview.CC, ", ")) + fmt.Fprintf(w, "BCC: %s\n", strings.Join(preview.BCC, ", ")) + fmt.Fprintf(w, "Subject: %s\n", preview.Subject) + for _, attachment := range preview.Attachments { + fmt.Fprintf(w, "Attachment: %s (%s, %d bytes)\n", attachment.Path, attachment.ContentType, attachment.ByteSize) + } + fmt.Fprintln(w) + fmt.Fprintln(w, preview.Body) + return nil + } + + return writeOK(preview, output.WithSummary("Reply preview")) +} + +func defaultReplySenderEmail(ctx context.Context) (string, error) { + identity, err := sdk.Identity().GetIdentity(ctx) + if err != nil { + return "", convertSDKError(err) + } + if identity == nil { + return "", output.ErrAPI(0, "could not determine sender identity") + } + for _, sender := range identity.Senders { + if sender.Default && sender.EmailAddress != "" { + return sender.EmailAddress, nil + } + } + for _, sender := range identity.Senders { + if sender.EmailAddress != "" { + return sender.EmailAddress, nil + } + } + if identity.PrimaryContact.EmailAddress != "" { + return identity.PrimaryContact.EmailAddress, nil + } + return "", output.ErrAPI(0, "could not determine sender email address") +} + +func defaultReplyVerificationDelays() []time.Duration { + return []time.Duration{ + 0, + 250 * time.Millisecond, + 500 * time.Millisecond, + 1 * time.Second, + 2 * time.Second, + 4 * time.Second, + 8 * time.Second, + } +} + +func verifyReplyCreated(ctx context.Context, threadID, previousEntryID, senderID int64, sentContent string, delays []time.Duration) (int64, error) { + var lastErr error + for _, delay := range delays { + if delay > 0 { + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + return 0, output.ErrAPI(0, "HEY accepted the reply, but verification was canceled. Do not retry automatically; check the thread and Sent Mail") + case <-timer.C: + } + } + + entryID, err := findMatchingReply(ctx, threadID, previousEntryID, senderID, sentContent) + if err != nil { + lastErr = convertSDKError(err) + continue + } + lastErr = nil + if entryID > 0 { + return entryID, nil + } + } + + if lastErr != nil { + return 0, output.ErrAPI(0, fmt.Sprintf("HEY accepted the reply, but the sent entry could not be verified: %v. Do not retry automatically; check the thread and Sent Mail", lastErr)) + } + return 0, output.ErrAPI(0, "HEY accepted the reply, but no matching sent entry appeared. Do not retry automatically; check the thread and Sent Mail") +} + +func findMatchingReply(ctx context.Context, threadID, previousEntryID, senderID int64, sentContent string) (int64, error) { + var candidateErr error + for page := 1; page <= maxReplyVerificationPages; page++ { + pageValue := strconv.Itoa(page) + entries, err := sdk.Topics().GetEntries(ctx, threadID, &generated.GetTopicEntriesParams{Page: &pageValue}) + if err != nil { + return 0, err + } + if entries == nil || len(*entries) == 0 { + if candidateErr != nil { + return 0, candidateErr + } + return 0, nil + } + for _, entry := range *entries { + if entry.Id <= previousEntryID || (entry.Kind != "" && entry.Kind != "message") { + continue + } + message, err := sdk.Messages().Get(ctx, entry.Id) + if err != nil { + candidateErr = err + continue + } + if replyMessageMatches(entry, message, senderID, sentContent) { + return entry.Id, nil + } + } + } + return 0, fmt.Errorf("reply verification stopped after %d pages", maxReplyVerificationPages) +} + +func replyMessageMatches(entry generated.Entry, message *generated.Message, senderID int64, sentContent string) bool { + if message == nil { + return false + } + fromSender := entry.Creator.Id == senderID || message.Creator.Id == senderID || message.Sender.Id == senderID + if !fromSender { + return false + } + expected := normalizedReplyContent(sentContent) + actual := normalizedReplyContent(message.Content) + return expected != "" && (actual == expected || strings.HasPrefix(actual, expected+" ")) +} + +func normalizedReplyContent(content string) string { + return strings.Join(strings.Fields(htmlutil.ToText(content)), " ") +} diff --git a/internal/cmd/reply_test.go b/internal/cmd/reply_test.go new file mode 100644 index 00000000..1183cd4e --- /dev/null +++ b/internal/cmd/reply_test.go @@ -0,0 +1,280 @@ +package cmd + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + "time" + + "github.com/basecamp/hey-cli/internal/output" +) + +func runReply(t *testing.T, server *httptest.Server, args ...string) (output.Response, error) { + t.Helper() + t.Setenv("HEY_TOKEN", "test-token") + t.Setenv("HEY_NO_KEYRING", "1") + t.Setenv("HEY_BASE_URL", "") + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + t.Setenv("XDG_STATE_HOME", tmpDir) + t.Setenv("XDG_CACHE_HOME", tmpDir) + + root := newRootCmd() + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs(append([]string{"reply", "--json", "--base-url", server.URL}, args...)) + + err := root.Execute() + var response output.Response + if buf.Len() > 0 { + if decodeErr := json.Unmarshal(buf.Bytes(), &response); decodeErr != nil { + t.Fatalf("decode response: %v\n%s", decodeErr, buf.String()) + } + } + return response, err +} + +func TestReplyUsesLiveEnvelopeAndVerifiesCreatedEntry(t *testing.T) { + server, sent := threadReplyServer(t, replyForm) + + response, err := runReply(t, server, "7", "-m", "Thanks for the update.", "--expect-entry", "12") + if err != nil { + t.Fatalf("execute: %v", err) + } + if response.Summary != "Reply sent" { + t.Errorf("summary = %q", response.Summary) + } + data, ok := response.Data.(map[string]any) + if !ok || data["thread_id"] != float64(7) || data["entry_id"] != float64(13) { + t.Errorf("data = %#v", response.Data) + } + if sent.Content != "Thanks for the update." { + t.Errorf("content = %q", sent.Content) + } + if len(sent.To) != 1 || sent.To[0] != "jane@example.com" { + t.Errorf("to = %#v", sent.To) + } + if len(sent.CC) != 1 || sent.CC[0] != "cc@example.com" { + t.Errorf("cc = %#v", sent.CC) + } +} + +func TestReplyPreviewShowsEnvelopeAndAttachmentsWithoutSending(t *testing.T) { + server, sent := threadReplyServer(t, replyForm) + path := filepath.Join(t.TempDir(), "report.txt") + if err := os.WriteFile(path, []byte("report contents"), 0o600); err != nil { + t.Fatal(err) + } + + response, err := runReply(t, server, "7", "-m", "Preview body", "--attach", path, "--preview") + if err != nil { + t.Fatalf("execute: %v", err) + } + if sent.Path != "" { + t.Fatalf("preview sent a request to %q", sent.Path) + } + if response.Summary != "Reply preview" { + t.Errorf("summary = %q", response.Summary) + } + data, ok := response.Data.(map[string]any) + if !ok { + t.Fatalf("data = %#v", response.Data) + } + if data["from"] != "user@hey.com" || data["subject"] != "Re: Project update" || data["body"] != "Preview body" { + t.Errorf("preview data = %#v", data) + } + if data["entry_id"] != float64(12) { + t.Errorf("preview entry_id = %#v, want 12", data["entry_id"]) + } + attachments, ok := data["attachments"].([]any) + if !ok || len(attachments) != 1 { + t.Fatalf("attachments = %#v", data["attachments"]) + } + attachment, ok := attachments[0].(map[string]any) + if !ok || attachment["path"] != path || attachment["filename"] != "report.txt" || attachment["byte_size"] != float64(15) { + t.Errorf("attachment = %#v", attachments[0]) + } +} + +func TestReplyRequiresPreviewedEntry(t *testing.T) { + server, sent := threadReplyServer(t, replyForm) + + _, err := runReply(t, server, "7", "-m", "Unbound reply") + if err == nil || !strings.Contains(err.Error(), "requires --expect-entry") { + t.Fatalf("unbound send error = %v", err) + } + if sent.Path != "" { + t.Fatalf("unbound send wrote to %q", sent.Path) + } + + _, err = runReply(t, server, "7", "-m", "Invalid target", "--expect-entry", "0") + if err == nil || !strings.Contains(err.Error(), "positive entry ID") { + t.Fatalf("invalid target error = %v", err) + } + if sent.Path != "" { + t.Fatalf("invalid target wrote to %q", sent.Path) + } + + _, err = runReply(t, server, "7", "-m", "Stale reply", "--expect-entry", "11") + if err == nil || !strings.Contains(err.Error(), "thread changed after preview") { + t.Fatalf("stale send error = %v", err) + } + if sent.Path != "" { + t.Fatalf("stale send wrote to %q", sent.Path) + } +} + +func TestVerifyReplyCreatedRejectsUnchangedTopic(t *testing.T) { + server := replyVerificationServer(t, []verificationCandidate{}) + withSDKPointedAt(t, server) + + _, err := verifyReplyCreated(context.Background(), 7, 12, 42, "Reply body", []time.Duration{0}) + if err == nil || !strings.Contains(err.Error(), "no matching sent entry appeared") || !strings.Contains(err.Error(), "Do not retry automatically") { + t.Fatalf("error = %v, want unverified-send safety error", err) + } +} + +func TestVerifyReplyCreatedWaitsForPropagation(t *testing.T) { + server := replyVerificationServer(t, + []verificationCandidate{}, + []verificationCandidate{{ID: 13, SenderID: 42, Content: "
Reply body
"}}, + ) + withSDKPointedAt(t, server) + + entryID, err := verifyReplyCreated(context.Background(), 7, 12, 42, "Reply body", []time.Duration{0, 0}) + if err != nil { + t.Fatalf("verify: %v", err) + } + if entryID != 13 { + t.Errorf("entry = %d, want 13", entryID) + } +} + +func TestVerifyReplyCreatedRejectsAnotherParticipantsEntry(t *testing.T) { + server := replyVerificationServer(t, + []verificationCandidate{{ID: 13, SenderID: 99, Content: "Reply body"}}, + ) + withSDKPointedAt(t, server) + + _, err := verifyReplyCreated(context.Background(), 7, 12, 42, "Reply body", []time.Duration{0}) + if err == nil || !strings.Contains(err.Error(), "no matching sent entry appeared") { + t.Fatalf("error = %v, want unrelated entry rejection", err) + } +} + +func TestVerifyReplyCreatedFindsReplyAfterUnrelatedEntry(t *testing.T) { + server := replyVerificationServer(t, + []verificationCandidate{ + {ID: 13, SenderID: 99, Content: "Reply body"}, + {ID: 14, SenderID: 42, Content: "

Reply body

"}, + }, + ) + withSDKPointedAt(t, server) + + entryID, err := verifyReplyCreated(context.Background(), 7, 12, 42, "Reply body", []time.Duration{0}) + if err != nil { + t.Fatalf("verify: %v", err) + } + if entryID != 14 { + t.Errorf("entry = %d, want 14", entryID) + } +} + +func TestVerifyReplyCreatedRejectsDifferentContentFromCurrentUser(t *testing.T) { + server := replyVerificationServer(t, + []verificationCandidate{{ID: 13, SenderID: 42, Content: "Different message"}}, + ) + withSDKPointedAt(t, server) + + _, err := verifyReplyCreated(context.Background(), 7, 12, 42, "Reply body", []time.Duration{0}) + if err == nil || !strings.Contains(err.Error(), "no matching sent entry appeared") { + t.Fatalf("error = %v, want content mismatch rejection", err) + } +} + +func TestReplyVerificationWindowCoversSlowPropagation(t *testing.T) { + var window time.Duration + for _, delay := range defaultReplyVerificationDelays() { + window += delay + } + if window < 10*time.Second { + t.Errorf("verification window = %s, want at least 10s", window) + } +} + +type verificationCandidate struct { + ID int64 + SenderID int64 + Content string +} + +func replyVerificationServer(t *testing.T, batches ...[]verificationCandidate) *httptest.Server { + t.Helper() + reads := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + switch { + case r.URL.Path == "/topics/7/entries.json": + if r.URL.Query().Get("page") != "1" { + _, _ = fmt.Fprint(w, `[]`) + return + } + index := reads + if index >= len(batches) { + index = len(batches) - 1 + } + reads++ + if index < 0 { + _, _ = fmt.Fprint(w, `[]`) + return + } + entries := make([]map[string]any, 0, len(batches[index])) + for _, candidate := range batches[index] { + entries = append(entries, map[string]any{ + "id": candidate.ID, + "kind": "message", + "creator": map[string]any{"id": candidate.SenderID}, + }) + } + _ = json.NewEncoder(w).Encode(entries) + case strings.HasPrefix(r.URL.Path, "/messages/") && strings.HasSuffix(r.URL.Path, ".json"): + messageID, err := strconv.ParseInt(strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/messages/"), ".json"), 10, 64) + if err != nil { + t.Fatalf("parse message ID: %v", err) + } + for _, batch := range batches { + for _, candidate := range batch { + if candidate.ID == messageID { + _ = json.NewEncoder(w).Encode(map[string]any{ + "id": candidate.ID, + "content": candidate.Content, + "creator": map[string]any{"id": candidate.SenderID}, + "sender": map[string]any{"id": candidate.SenderID}, + }) + return + } + } + } + http.NotFound(w, r) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.NotFound(w, r) + } + })) + t.Cleanup(server.Close) + return server +} diff --git a/internal/cmd/thread_reply.go b/internal/cmd/thread_reply.go index 36bdd1b5..919ae3a6 100644 --- a/internal/cmd/thread_reply.go +++ b/internal/cmd/thread_reply.go @@ -13,33 +13,45 @@ import ( // the recipients are not optional. type threadReplyTarget struct { EntryID int64 - Addressed *htmlutil.TopicAddressed + Subject string + Addressed *htmlutil.ReplyForm } // resolveThreadReply works out what replying to a thread means: the last entry on it, and -// who that entry went to. -// -// It still reads the topic's HTML pages. The SDK gained typed reads for both in v0.4.0 — -// Topics.Get carries the topic's entries — and this is the one place to change when the -// CLI moves onto them. +// the live envelope HEY prepared for that reply. func resolveThreadReply(ctx context.Context, threadID int64) (*threadReplyTarget, error) { - topicResp, err := sdk.GetHTML(ctx, fmt.Sprintf("/topics/%d", threadID)) + return resolveThreadReplyAtEntry(ctx, threadID, 0) +} + +// resolveThreadReplyAtEntry refuses to load a reply envelope when the latest entry no +// longer matches the one the user previewed. +func resolveThreadReplyAtEntry(ctx context.Context, threadID, expectedEntryID int64) (*threadReplyTarget, error) { + topic, err := sdk.Topics().Get(ctx, threadID) if err != nil { return nil, convertSDKError(err) } - addressed := htmlutil.ParseTopicAddressed(string(topicResp.Data)) - if len(addressed.To) == 0 && len(addressed.CC) == 0 && len(addressed.BCC) == 0 { - return nil, output.ErrUsage("could not determine thread recipients") + if topic == nil || topic.LatestEntry.Id <= 0 { + return nil, output.ErrNotFound("entries for thread", fmt.Sprintf("%d", threadID)) + } + if expectedEntryID > 0 && topic.LatestEntry.Id != expectedEntryID { + return nil, output.ErrUsageHint( + fmt.Sprintf("thread changed after preview: expected entry %d, latest entry is %d", expectedEntryID, topic.LatestEntry.Id), + fmt.Sprintf("Run: hey reply %d -m --preview --json", threadID), + ) } - entriesResp, err := sdk.GetHTML(ctx, fmt.Sprintf("/topics/%d/entries", threadID)) + replyResp, err := sdk.GetHTML(ctx, fmt.Sprintf("/entries/%d/replies/new", topic.LatestEntry.Id)) if err != nil { return nil, convertSDKError(err) } - entries := htmlutil.ParseTopicEntriesHTML(string(entriesResp.Data)) - if len(entries) == 0 { - return nil, output.ErrNotFound("entries for thread", fmt.Sprintf("%d", threadID)) + addressed, err := htmlutil.ParseReplyFormHTML(string(replyResp.Data)) + if err != nil { + return nil, output.ErrUsage("could not determine thread recipients from HEY's reply form") } - return &threadReplyTarget{EntryID: entries[len(entries)-1].ID, Addressed: addressed}, nil + return &threadReplyTarget{ + EntryID: topic.LatestEntry.Id, + Subject: topic.Name, + Addressed: &addressed, + }, nil } diff --git a/internal/cmd/thread_reply_test.go b/internal/cmd/thread_reply_test.go index 1c8c5cda..fa6c3b51 100644 --- a/internal/cmd/thread_reply_test.go +++ b/internal/cmd/thread_reply_test.go @@ -8,19 +8,16 @@ import ( "fmt" "net/http" "net/http/httptest" - "strings" "testing" "github.com/basecamp/hey-cli/internal/apierr" ) -const ( - topicWithRecipients = ` - Jane - CC: Cee - ` - topicEntries = `
` -) +const replyForm = "
" + + "" + + "" + + "" + + "
" // sentReply is what the server saw a reply arrive as. type sentReply struct { @@ -31,15 +28,37 @@ type sentReply struct { BCC []string } -// threadReplyServer answers the two topic pages resolveThreadReply reads, the identity -// the SDK needs for a sending operation, and the reply itself — recording it so a test -// can say what actually went out. -func threadReplyServer(t *testing.T, topicHTML, entriesHTML string) (*httptest.Server, *sentReply) { +// threadReplyServer answers the typed topic, live reply form, identity, and reply +// requests used by reply and compose tests. +func threadReplyServer(t *testing.T, formHTML string) (*httptest.Server, *sentReply) { t.Helper() sent := &sentReply{} + latestEntryID := int64(12) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { - case strings.Contains(r.URL.Path, "/replies"): + case r.Method == http.MethodGet && (r.URL.Path == "/topics/7" || r.URL.Path == "/topics/7.json"): + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, "{\"id\":7,\"name\":\"Project update\",\"latest_entry\":{\"id\":%d}}", latestEntryID) + case r.Method == http.MethodGet && r.URL.Path == "/entries/12/replies/new": + w.Header().Set("Content-Type", "text/html") + _, _ = fmt.Fprint(w, formHTML) + case r.Method == http.MethodGet && r.URL.Path == "/topics/7/entries.json": + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "1" && latestEntryID == 13 { + _, _ = fmt.Fprint(w, `[{"id":13,"kind":"message","creator":{"id":42,"email_address":"user@hey.com"}}]`) + } else { + _, _ = fmt.Fprint(w, `[]`) + } + case r.Method == http.MethodGet && r.URL.Path == "/messages/13.json": + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "id": 13, + "content": sent.Content, + "is_reply": true, + "creator": map[string]any{"id": 42, "email_address": "user@hey.com"}, + "sender": map[string]any{"id": 42, "email_address": "user@hey.com"}, + }) + case r.Method == http.MethodPost && r.URL.Path == "/entries/12/replies.json": var body struct { Message struct { Content string `json:"content"` @@ -52,22 +71,24 @@ func threadReplyServer(t *testing.T, topicHTML, entriesHTML string) (*httptest.S } `json:"addressed"` } `json:"entry"` } - _ = json.NewDecoder(r.Body).Decode(&body) + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode reply: %v", err) + } sent.Path = r.URL.Path sent.Content = body.Message.Content - sent.To, sent.CC, sent.BCC = body.Entry.Addressed.Directly, body.Entry.Addressed.Copied, body.Entry.Addressed.Blindcopied + sent.To = body.Entry.Addressed.Directly + sent.CC = body.Entry.Addressed.Copied + sent.BCC = body.Entry.Addressed.Blindcopied + latestEntryID = 13 w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated) - fmt.Fprint(w, `{}`) - case strings.Contains(r.URL.Path, "identity"): + _, _ = fmt.Fprint(w, "{}") + case r.Method == http.MethodGet && r.URL.Path == "/identity.json": w.Header().Set("Content-Type", "application/json") - fmt.Fprint(w, `{"id":1,"senders":[{"id":42,"default":true}]}`) - case strings.HasSuffix(r.URL.Path, "/entries"): - w.Header().Set("Content-Type", "text/html") - fmt.Fprint(w, entriesHTML) + _, _ = fmt.Fprint(w, "{\"id\":1,\"senders\":[{\"id\":42,\"default\":true,\"email_address\":\"user@hey.com\"}],\"primary_contact\":{\"id\":42,\"email_address\":\"user@hey.com\"}}") default: - w.Header().Set("Content-Type", "text/html") - fmt.Fprint(w, topicHTML) + t.Errorf("unexpected request: %s %s", r.Method, r.URL.RequestURI()) + http.NotFound(w, r) } })) t.Cleanup(server.Close) @@ -86,8 +107,8 @@ func withSDKPointedAt(t *testing.T, server *httptest.Server) { initSDK(nil, server.URL) } -func TestResolveThreadReply(t *testing.T) { - server, _ := threadReplyServer(t, topicWithRecipients, topicEntries) +func TestResolveThreadReplyUsesLatestEntryAndLiveEnvelope(t *testing.T) { + server, _ := threadReplyServer(t, replyForm) withSDKPointedAt(t, server) target, err := resolveThreadReply(context.Background(), 7) @@ -95,9 +116,11 @@ func TestResolveThreadReply(t *testing.T) { t.Fatalf("unexpected error: %v", err) } - // HEY replies to the last entry on the thread, not the first. if target.EntryID != 12 { - t.Errorf("entry = %d, want the last one (12)", target.EntryID) + t.Errorf("entry = %d, want 12", target.EntryID) + } + if target.Subject != "Project update" { + t.Errorf("subject = %q", target.Subject) } if len(target.Addressed.To) != 1 || target.Addressed.To[0] != "jane@example.com" { t.Errorf("to = %v", target.Addressed.To) @@ -107,10 +130,10 @@ func TestResolveThreadReply(t *testing.T) { } } -// An unaddressed reply is saved as a draft rather than sent, so a thread we cannot read -// recipients from is refused before anything is written. +// An unaddressed reply is saved as a draft rather than sent, so a thread whose live +// reply form has no recipients is refused before anything is written. func TestResolveThreadReplyWithoutRecipients(t *testing.T) { - server, _ := threadReplyServer(t, `no recipients here`, topicEntries) + server, _ := threadReplyServer(t, "
") withSDKPointedAt(t, server) _, err := resolveThreadReply(context.Background(), 7) @@ -122,7 +145,11 @@ func TestResolveThreadReplyWithoutRecipients(t *testing.T) { } func TestResolveThreadReplyWithoutEntries(t *testing.T) { - server, _ := threadReplyServer(t, topicWithRecipients, ``) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, "{\"id\":7,\"name\":\"Empty\",\"latest_entry\":{}}") + })) + t.Cleanup(server.Close) withSDKPointedAt(t, server) _, err := resolveThreadReply(context.Background(), 7) @@ -133,8 +160,8 @@ func TestResolveThreadReplyWithoutEntries(t *testing.T) { } } -// runCLI drives a command the way the binary does — through the root command, so the -// output writer and auth are set up — against a test server. +// runCLI drives a command the way the binary does through the root command, so output +// and authentication are configured against the test server. func runCLI(t *testing.T, server *httptest.Server, args ...string) error { t.Helper() t.Setenv("HEY_TOKEN", "test-token") diff --git a/internal/cmd/topic.go b/internal/cmd/topic.go index aca32e36..e9fb01cc 100644 --- a/internal/cmd/topic.go +++ b/internal/cmd/topic.go @@ -86,8 +86,8 @@ func (c *topicCommand) run(cmd *cobra.Command, args []string) error { output.WithBreadcrumbs( output.Breadcrumb{ Action: "reply", - Command: fmt.Sprintf("hey reply %d", threadID), - Description: "Reply to this thread", + Command: fmt.Sprintf("hey reply %d --preview", threadID), + Description: "Preview a reply to this thread", }, output.Breadcrumb{ Action: "forward", diff --git a/internal/htmlutil/reply.go b/internal/htmlutil/reply.go new file mode 100644 index 00000000..ac094cd3 --- /dev/null +++ b/internal/htmlutil/reply.go @@ -0,0 +1,126 @@ +package htmlutil + +import ( + "fmt" + "net/url" + "strconv" + "strings" + + "golang.org/x/net/html" +) + +// ReplyForm is the envelope HEY prepares for replying to an entry. +type ReplyForm struct { + To []string + CC []string + BCC []string +} + +// ParseReplyFormHTML extracts the recipients from HEY's reply form. +func ParseReplyFormHTML(pageHTML string) (ReplyForm, error) { + doc, err := html.Parse(strings.NewReader(pageHTML)) + if err != nil { + return ReplyForm{}, fmt.Errorf("parse reply page: %w", err) + } + + form := findReplyForm(doc) + if form == nil { + return ReplyForm{}, fmt.Errorf("reply page does not contain an entry reply form") + } + + to := replyFormAddresses(form, "entry[addressed][directly][]") + cc := replyFormAddresses(form, "entry[addressed][copied][]") + bcc := replyFormAddresses(form, "entry[addressed][blindcopied][]") + if len(to)+len(cc)+len(bcc) == 0 { + return ReplyForm{}, fmt.Errorf("reply form does not contain recipients") + } + + return ReplyForm{To: to, CC: cc, BCC: bcc}, nil +} + +func findReplyForm(node *html.Node) *html.Node { + if node.Type == html.ElementNode && node.Data == "form" { + action := getAttr(node, "action") + if parsed, err := url.Parse(action); err == nil && isReplyFormPath(parsed.Path) { + return node + } + } + + for child := node.FirstChild; child != nil; child = child.NextSibling { + if form := findReplyForm(child); form != nil { + return form + } + } + return nil +} + +func isReplyFormPath(path string) bool { + parts := strings.Split(strings.Trim(path, "/"), "/") + if len(parts) != 3 || parts[0] != "entries" || parts[2] != "replies" { + return false + } + entryID, err := strconv.ParseInt(parts[1], 10, 64) + return err == nil && entryID > 0 +} + +func replyFormAddresses(node *html.Node, name string) []string { + var inputValues, selectValues []string + selectFound := false + var walk func(*html.Node) + walk = func(current *html.Node) { + if current.Type == html.ElementNode && getAttr(current, "name") == name { + switch current.Data { + case "select": + if !selectFound { + selectFound = true + selectValues = replyOptionValues(current) + } + case "input": + if value := strings.TrimSpace(getAttr(current, "value")); value != "" { + inputValues = append(inputValues, value) + } + } + } + for child := current.FirstChild; child != nil; child = child.NextSibling { + walk(child) + } + } + walk(node) + if selectFound && len(selectValues) > 0 { + return selectValues + } + return inputValues +} + +func replyOptionValues(selectNode *html.Node) []string { + var all, selected []string + var walk func(*html.Node) + walk = func(node *html.Node) { + if node.Type == html.ElementNode && node.Data == "option" { + value := strings.TrimSpace(getAttr(node, "value")) + if value != "" { + all = append(all, value) + if hasAttr(node, "selected") { + selected = append(selected, value) + } + } + } + for child := node.FirstChild; child != nil; child = child.NextSibling { + walk(child) + } + } + walk(selectNode) + if len(selected) > 0 { + return selected + } + return all +} + +func hasAttr(node *html.Node, key string) bool { + for _, attr := range node.Attr { + if attr.Key == key { + return true + } + } + return false +} diff --git a/internal/htmlutil/reply_test.go b/internal/htmlutil/reply_test.go new file mode 100644 index 00000000..61d1eb0f --- /dev/null +++ b/internal/htmlutil/reply_test.go @@ -0,0 +1,80 @@ +package htmlutil + +import ( + "reflect" + "strings" + "testing" +) + +func TestParseReplyFormHTML(t *testing.T) { + page := ` +
+
+ + + +
` + + form, err := ParseReplyFormHTML(page) + if err != nil { + t.Fatalf("parse: %v", err) + } + if !reflect.DeepEqual(form.To, []string{"alice@example.com"}) { + t.Errorf("to = %#v", form.To) + } + if !reflect.DeepEqual(form.CC, []string{"carol@example.com"}) { + t.Errorf("cc = %#v", form.CC) + } + if len(form.BCC) != 0 { + t.Errorf("bcc = %#v", form.BCC) + } +} + +func TestParseReplyFormHTMLSupportsAbsoluteActionAndHiddenRecipients(t *testing.T) { + page := `
+ + +
` + + form, err := ParseReplyFormHTML(page) + if err != nil { + t.Fatalf("parse: %v", err) + } + if !reflect.DeepEqual(form.To, []string{"alice@example.com", "bob@example.org"}) { + t.Errorf("to = %#v", form.To) + } +} + +func TestParseReplyFormHTMLFallsBackToHiddenRecipientsWhenSelectIsEmpty(t *testing.T) { + page := `
+ + +
` + + form, err := ParseReplyFormHTML(page) + if err != nil { + t.Fatalf("parse: %v", err) + } + if !reflect.DeepEqual(form.To, []string{"alice@example.com"}) { + t.Errorf("to = %#v", form.To) + } +} + +func TestParseReplyFormHTMLRejectsMissingReplyForm(t *testing.T) { + _, err := ParseReplyFormHTML(`
`) + if err == nil || !strings.Contains(err.Error(), "entry reply form") { + t.Fatalf("error = %v, want missing form error", err) + } +} + +func TestParseReplyFormHTMLRejectsMissingRecipients(t *testing.T) { + _, err := ParseReplyFormHTML(`
`) + if err == nil || !strings.Contains(err.Error(), "recipients") { + t.Fatalf("error = %v, want missing recipients error", err) + } +} diff --git a/skills/hey/SKILL.md b/skills/hey/SKILL.md index 61c3e4e6..96a004b5 100644 --- a/skills/hey/SKILL.md +++ b/skills/hey/SKILL.md @@ -98,6 +98,7 @@ CLI for HEY: mailboxes, email threads, contacts, replies, compose, calendars, to 1. **Always use `--json`** for structured, predictable output 2. **Authentication required** for all data commands — run `hey auth login` first 3. **HTML output** is available via `--html` for commands that return HTML content +4. **Preview every outbound email.** Show From, To, CC, BCC, Subject, the complete body, and attachments, then obtain explicit confirmation immediately before sending. ## Quick Reference @@ -117,7 +118,8 @@ CLI for HEY: mailboxes, email threads, contacts, replies, compose, calendars, to | Set private contact note | `hey contacts note set "Prefers email"` | | Delete private contact note | `hey contacts note delete ` | | Read email thread | `hey threads --json` | -| Reply to email | `hey reply -m "Thanks!"` | +| Preview a reply | `hey reply -m "Thanks!" --preview --json` | +| Send a reply | `hey reply -m "Thanks!" --expect-entry ` | | Forward email | `hey forward --to alice@example.com -m "For your review"` | | Compose email | `hey compose --to user@example.com --subject "Hello"` | | Compose with CC/BCC | `hey compose --to alice@example.com --cc bob@example.com --bcc carol@example.org --subject "Hello"` | @@ -175,8 +177,9 @@ Want to read email? ``` Want to send email? -├── Reply to thread? → hey reply -m "message" -│ ├── Open editor? → hey reply (omit -m to open $EDITOR) +├── Reply to thread? → hey reply -m "message" --preview --json +│ ├── Send after confirmation? → pass the preview's entry_id to --expect-entry +│ ├── Open editor? → hey reply --expect-entry (omit -m to open $EDITOR) │ └── Attach files? → add --attach ./report.pdf (repeatable) ├── Forward latest message? → hey forward --to │ └── Add a note? → add -m "note" @@ -273,9 +276,10 @@ An attachment ID combines its message ID and position, so `67890:1` identifies t ### Email - Reply, Forward & Compose ```bash -hey reply -m "Thanks!" # Reply with inline message -hey reply # Reply via $EDITOR -hey reply -m "Attached." --attach ./diagram.png +hey reply -m "Thanks!" --preview --json # Preview complete reply +hey reply -m "Thanks!" --expect-entry 67890 --json # Send the previewed reply +hey reply --expect-entry 67890 # Reply via $EDITOR +hey reply -m "Attached." --attach ./diagram.png --preview hey forward --to alice@example.com # Forward the latest message hey forward --to alice@example.com -m "Please review" # Forward with a note hey compose --to user@example.com --subject "Hello" # Compose new (opens $EDITOR) diff --git a/tests/smoke/compose_test.go b/tests/smoke/compose_test.go index e62d6dbf..d3b69c05 100644 --- a/tests/smoke/compose_test.go +++ b/tests/smoke/compose_test.go @@ -129,9 +129,20 @@ func TestReply(t *testing.T) { t.Fatal("could not find a thread to reply to") } - // Reply to it. + message := fmt.Sprintf("Reply from smoke test %s", uid) + preview := heyJSON(t, "reply", topicID, "-m", message, "--preview") + type replyPreview struct { + EntryID int64 `json:"entry_id"` + } + previewData := dataAs[replyPreview](t, preview) + if previewData.EntryID <= 0 { + t.Fatal("reply preview did not return an entry_id") + } + + // Reply to exactly the entry that was previewed. stdout, stderr, code := hey(t, "reply", topicID, - "-m", fmt.Sprintf("Reply from smoke test %s", uid), + "-m", message, + "--expect-entry", fmt.Sprint(previewData.EntryID), "--json", ) if code != 0 { From 19c8ff3ba036c06190addae9c11c207ac7540bff Mon Sep 17 00:00:00 2001 From: Ellis Givens Date: Wed, 19 Aug 2026 14:25:36 -0500 Subject: [PATCH 2/3] Tighten reply verification guidance --- internal/cmd/reply.go | 2 +- internal/cmd/reply_test.go | 12 ++++++++++++ skills/hey/SKILL.md | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/cmd/reply.go b/internal/cmd/reply.go index cc7928a6..e50ea27c 100644 --- a/internal/cmd/reply.go +++ b/internal/cmd/reply.go @@ -348,7 +348,7 @@ func replyMessageMatches(entry generated.Entry, message *generated.Message, send } expected := normalizedReplyContent(sentContent) actual := normalizedReplyContent(message.Content) - return expected != "" && (actual == expected || strings.HasPrefix(actual, expected+" ")) + return expected != "" && actual == expected } func normalizedReplyContent(content string) string { diff --git a/internal/cmd/reply_test.go b/internal/cmd/reply_test.go index 1183cd4e..41235584 100644 --- a/internal/cmd/reply_test.go +++ b/internal/cmd/reply_test.go @@ -201,6 +201,18 @@ func TestVerifyReplyCreatedRejectsDifferentContentFromCurrentUser(t *testing.T) } } +func TestVerifyReplyCreatedRejectsLongerContentWithMatchingPrefix(t *testing.T) { + server := replyVerificationServer(t, + []verificationCandidate{{ID: 13, SenderID: 42, Content: "Reply body with an unrelated ending"}}, + ) + withSDKPointedAt(t, server) + + _, err := verifyReplyCreated(context.Background(), 7, 12, 42, "Reply body", []time.Duration{0}) + if err == nil || !strings.Contains(err.Error(), "no matching sent entry appeared") { + t.Fatalf("error = %v, want prefix-only content rejection", err) + } +} + func TestReplyVerificationWindowCoversSlowPropagation(t *testing.T) { var window time.Duration for _, delay := range defaultReplyVerificationDelays() { diff --git a/skills/hey/SKILL.md b/skills/hey/SKILL.md index 96a004b5..5f320a79 100644 --- a/skills/hey/SKILL.md +++ b/skills/hey/SKILL.md @@ -98,7 +98,7 @@ CLI for HEY: mailboxes, email threads, contacts, replies, compose, calendars, to 1. **Always use `--json`** for structured, predictable output 2. **Authentication required** for all data commands — run `hey auth login` first 3. **HTML output** is available via `--html` for commands that return HTML content -4. **Preview every outbound email.** Show From, To, CC, BCC, Subject, the complete body, and attachments, then obtain explicit confirmation immediately before sending. +4. **Preview every reply.** Run `hey reply ... --preview --json`, show From, To, CC, BCC, Subject, the complete body, and attachments, then obtain explicit confirmation immediately before sending with `--expect-entry`. ## Quick Reference From 04b8d12f51ea87a239f0591745b7805127af8f33 Mon Sep 17 00:00:00 2001 From: Ellis Givens Date: Wed, 19 Aug 2026 14:41:07 -0500 Subject: [PATCH 3/3] Recheck reply target before delivery --- internal/cmd/reply.go | 20 ++++++++++++++++++++ internal/cmd/reply_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/internal/cmd/reply.go b/internal/cmd/reply.go index e50ea27c..8a56ee88 100644 --- a/internal/cmd/reply.go +++ b/internal/cmd/reply.go @@ -119,6 +119,9 @@ func (c *replyCommand) run(cmd *cobra.Command, args []string) error { return err } } + if err = ensureExpectedEntryStillLatest(ctx, threadID, c.expectedEntryID); err != nil { + return err + } if err = sdk.Entries().CreateReply(ctx, target.EntryID, message, target.Addressed.To, target.Addressed.CC, target.Addressed.BCC); err != nil { return convertSDKError(err) } @@ -150,6 +153,23 @@ func resolveExpectedThreadReply(ctx context.Context, threadID, expectedEntryID i return resolveThreadReplyAtEntry(ctx, threadID, expectedEntryID) } +func ensureExpectedEntryStillLatest(ctx context.Context, threadID, expectedEntryID int64) error { + topic, err := sdk.Topics().Get(ctx, threadID) + if err != nil { + return convertSDKError(err) + } + if topic == nil || topic.LatestEntry.Id <= 0 { + return output.ErrNotFound("entries for thread", fmt.Sprintf("%d", threadID)) + } + if topic.LatestEntry.Id != expectedEntryID { + return output.ErrUsageHint( + fmt.Sprintf("thread changed after preview: expected entry %d, latest entry is %d", expectedEntryID, topic.LatestEntry.Id), + fmt.Sprintf("Run: hey reply %d -m --preview --json", threadID), + ) + } + return nil +} + type replyPreviewAttachment struct { Path string `json:"path"` Filename string `json:"filename"` diff --git a/internal/cmd/reply_test.go b/internal/cmd/reply_test.go index 41235584..927b80ed 100644 --- a/internal/cmd/reply_test.go +++ b/internal/cmd/reply_test.go @@ -133,6 +133,39 @@ func TestReplyRequiresPreviewedEntry(t *testing.T) { } } +func TestReplyRechecksLatestEntryAfterLoadingEnvelope(t *testing.T) { + latestEntryID := int64(12) + posts := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && (r.URL.Path == "/topics/7" || r.URL.Path == "/topics/7.json"): + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"id":7,"name":"Project update","latest_entry":{"id":%d}}`, latestEntryID) + case r.Method == http.MethodGet && r.URL.Path == "/entries/12/replies/new": + latestEntryID = 13 + w.Header().Set("Content-Type", "text/html") + _, _ = fmt.Fprint(w, replyForm) + case r.Method == http.MethodGet && r.URL.Path == "/identity.json": + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, `{"id":1,"senders":[{"id":42,"default":true,"email_address":"user@hey.com"}],"primary_contact":{"id":42,"email_address":"user@hey.com"}}`) + case r.Method == http.MethodPost: + posts++ + w.WriteHeader(http.StatusCreated) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + _, err := runReply(t, server, "7", "-m", "Stale during envelope load", "--expect-entry", "12") + if err == nil || !strings.Contains(err.Error(), "thread changed after preview") { + t.Fatalf("error = %v, want stale-entry rejection", err) + } + if posts != 0 { + t.Fatalf("reply posts = %d, want 0", posts) + } +} + func TestVerifyReplyCreatedRejectsUnchangedTopic(t *testing.T) { server := replyVerificationServer(t, []verificationCandidate{}) withSDKPointedAt(t, server)