From f8688e0e331d3f53c1ca45385551ee82989691ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Victor?= Date: Mon, 29 Jun 2026 14:58:26 -0300 Subject: [PATCH 1/2] feat: support compose attachments --- .surface | 1 + README.md | 2 + internal/attachments/attachments.go | 216 +++++++++++++++++++++++ internal/attachments/attachments_test.go | 199 +++++++++++++++++++++ internal/cmd/compose.go | 56 +++++- internal/cmd/compose_attachments.go | 68 +++++++ internal/cmd/compose_attachments_test.go | 122 +++++++++++++ 7 files changed, 656 insertions(+), 8 deletions(-) create mode 100644 internal/attachments/attachments.go create mode 100644 internal/attachments/attachments_test.go create mode 100644 internal/cmd/compose_attachments.go create mode 100644 internal/cmd/compose_attachments_test.go diff --git a/.surface b/.surface index c688616..cfc8f6f 100644 --- a/.surface +++ b/.surface @@ -30,6 +30,7 @@ hey calendars hey commands hey completion hey compose +hey compose --attachment hey compose --bcc hey compose --cc hey compose --message diff --git a/README.md b/README.md index 8525306..9f56d6d 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,8 @@ hey threads 123 # read a full email thread hey reply 123 -m "Thanks!" # reply to a thread (or omit -m to open $EDITOR) hey compose --to user@example.com --subject "Hello" # compose a new message hey compose --to user@example.com --cc bob@example.com --bcc carol@example.org --subject "Hello" # with CC/BCC +hey compose --to user@example.com --subject "Q3 deck" -m "See attached" -a ~/reports/q3.pdf -a ~/charts/revenue.png # with attachments +hey compose --subject "Update" --thread-id 123 -m "Updated figures attached" -a ~/reports/q3-final.pdf # reply with an attachment hey drafts # list drafts ``` diff --git a/internal/attachments/attachments.go b/internal/attachments/attachments.go new file mode 100644 index 0000000..62631f9 --- /dev/null +++ b/internal/attachments/attachments.go @@ -0,0 +1,216 @@ +// Package attachments uploads local files through HEY's Active Storage +// direct-upload flow and renders the ActionText markup needed to embed +// them in a message body. +// +// The flow has three steps, matching Rails' direct-upload convention: +// +// 1. Create a direct upload by POSTing blob metadata (filename, byte size, +// MD5 checksum, content type) to the HEY API. The response describes +// where to PUT the bytes and how to reference the stored blob. +// 2. PUT the raw file bytes to the returned, self-authenticating URL. +// 3. Embed an element referencing the blob in the +// message content before sending. +package attachments + +import ( + "bytes" + "context" + "crypto/md5" //nolint:gosec // Active Storage requires an MD5 checksum, not for security + "encoding/base64" + "fmt" + "html" + "io" + "mime" + "net/http" + "os" + "path/filepath" + "strings" + + "github.com/basecamp/hey-cli/internal/apierr" +) + +// Blob is the metadata Active Storage needs to create a direct upload. +type Blob struct { + Filename string + ContentType string + ByteSize int64 + Checksum string // base64-encoded MD5 digest +} + +// DirectUpload describes where to upload the bytes and how to reference the +// blob once stored. It mirrors the JSON returned by Rails' direct-upload +// endpoint. +type DirectUpload struct { + SignedID string + AttachableSGID string + URL string + Headers map[string]string +} + +// Attachment is an uploaded file, ready to be embedded in message content. +type Attachment struct { + Filename string + ContentType string + ByteSize int64 + SignedID string + SGID string +} + +// DirectUploadCreator creates an Active Storage direct upload via the HEY API. +// Implementations route the request through the SDK client. +type DirectUploadCreator interface { + CreateDirectUpload(ctx context.Context, blob Blob) (*DirectUpload, error) +} + +// Validate reports whether path refers to a readable regular file. It returns +// a usage error for missing files, directories, and unreadable files so the +// caller can reject bad input before sending anything. +func Validate(path string) error { + info, err := os.Stat(path) + if err != nil { + if os.IsNotExist(err) { + return apierr.ErrUsage(fmt.Sprintf("attachment not found: %s", path)) + } + return apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + if info.IsDir() { + return apierr.ErrUsage(fmt.Sprintf("attachment is a directory, not a file: %s", path)) + } + if !info.Mode().IsRegular() { + return apierr.ErrUsage(fmt.Sprintf("attachment is not a regular file: %s", path)) + } + f, err := os.Open(path) //nolint:gosec // path is user-provided by design + if err != nil { + return apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + _ = f.Close() + return nil +} + +// AppendMarkup appends ActionText markup for each attachment to the message +// content. Content with no attachments is returned unchanged. +func AppendMarkup(content string, atts []*Attachment) string { + if len(atts) == 0 { + return content + } + var b strings.Builder + b.WriteString(content) + for _, att := range atts { + if b.Len() > 0 { + b.WriteString("\n") + } + b.WriteString(att.Markup()) + } + return b.String() +} + +// Uploader uploads local files through the Active Storage direct-upload flow. +type Uploader struct { + creator DirectUploadCreator + httpClient *http.Client +} + +// NewUploader returns an Uploader that creates direct uploads via creator and +// transfers blob bytes with httpClient. The blob PUT targets a +// self-authenticating URL, so httpClient needs no HEY credentials. +func NewUploader(creator DirectUploadCreator, httpClient *http.Client) *Uploader { + if httpClient == nil { + httpClient = http.DefaultClient + } + return &Uploader{creator: creator, httpClient: httpClient} +} + +// Upload validates path, creates a direct upload, transfers the bytes, and +// returns an Attachment describing the stored blob. +func (u *Uploader) Upload(ctx context.Context, path string) (*Attachment, error) { + if err := Validate(path); err != nil { + return nil, err + } + + data, err := os.ReadFile(path) //nolint:gosec // path is user-provided by design + if err != nil { + return nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + + blob := Blob{ + Filename: filepath.Base(path), + ContentType: detectContentType(path, data), + ByteSize: int64(len(data)), + Checksum: checksum(data), + } + + upload, err := u.creator.CreateDirectUpload(ctx, blob) + if err != nil { + return nil, err + } + + if err := u.put(ctx, upload, data); err != nil { + return nil, err + } + + return &Attachment{ + Filename: blob.Filename, + ContentType: blob.ContentType, + ByteSize: blob.ByteSize, + SignedID: upload.SignedID, + SGID: attachableSGID(upload), + }, nil +} + +func (u *Uploader) put(ctx context.Context, upload *DirectUpload, data []byte) error { + req, err := http.NewRequestWithContext(ctx, http.MethodPut, upload.URL, bytes.NewReader(data)) + if err != nil { + return apierr.ErrAPI(0, fmt.Sprintf("could not build upload request: %v", err)) + } + for k, v := range upload.Headers { + req.Header.Set(k, v) + } + + resp, err := u.httpClient.Do(req) + if err != nil { + return apierr.ErrNetwork(err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + return apierr.ErrAPI(resp.StatusCode, fmt.Sprintf("attachment upload failed (HTTP %d): %s", resp.StatusCode, strings.TrimSpace(string(body)))) + } + return nil +} + +// Markup returns safe ActionText markup embedding the attachment by its signed +// global ID. All attribute values are HTML-escaped. +func (a *Attachment) Markup() string { + return fmt.Sprintf( + ``, + html.EscapeString(a.SGID), + html.EscapeString(a.ContentType), + html.EscapeString(a.Filename), + a.ByteSize, + ) +} + +// attachableSGID prefers the attachable signed global ID, falling back to the +// blob's signed ID when the server omits it. +func attachableSGID(upload *DirectUpload) string { + if upload.AttachableSGID != "" { + return upload.AttachableSGID + } + return upload.SignedID +} + +func detectContentType(path string, data []byte) string { + if ct := mime.TypeByExtension(filepath.Ext(path)); ct != "" { + return ct + } + if ct := http.DetectContentType(data); ct != "application/octet-stream" { + return ct + } + return "application/octet-stream" +} + +func checksum(data []byte) string { + sum := md5.Sum(data) //nolint:gosec // Active Storage requires an MD5 checksum, not for security + return base64.StdEncoding.EncodeToString(sum[:]) +} diff --git a/internal/attachments/attachments_test.go b/internal/attachments/attachments_test.go new file mode 100644 index 0000000..4997d0d --- /dev/null +++ b/internal/attachments/attachments_test.go @@ -0,0 +1,199 @@ +package attachments + +import ( + "context" + "crypto/md5" + "encoding/base64" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeTempFile(t *testing.T, name string, data []byte) string { + t.Helper() + path := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("write temp file: %v", err) + } + return path +} + +func TestValidateMissingFile(t *testing.T) { + err := Validate(filepath.Join(t.TempDir(), "nope.png")) + if err == nil { + t.Fatal("expected error for missing file") + } +} + +func TestValidateDirectory(t *testing.T) { + err := Validate(t.TempDir()) + if err == nil { + t.Fatal("expected error for directory") + } +} + +func TestValidateRegularFile(t *testing.T) { + path := writeTempFile(t, "report.pdf", []byte("hello")) + if err := Validate(path); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestUploadFlow(t *testing.T) { + data := []byte("a tiny attachment payload") + path := writeTempFile(t, "diagram.png", data) + + wantChecksum := base64.StdEncoding.EncodeToString(md5Sum(data)) + + var gotBlob Blob + var putBody []byte + var putContentType string + + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + defer srv.Close() + + mux.HandleFunc("/upload/blob123", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPut { + t.Errorf("blob PUT method = %s, want PUT", r.Method) + } + putContentType = r.Header.Get("Content-Type") + b, _ := io.ReadAll(r.Body) + putBody = b + w.WriteHeader(http.StatusNoContent) + }) + + creator := creatorFunc(func(ctx context.Context, blob Blob) (*DirectUpload, error) { + gotBlob = blob + return &DirectUpload{ + SignedID: "signed-abc", + AttachableSGID: "sgid-xyz", + URL: srv.URL + "/upload/blob123", + Headers: map[string]string{"Content-Type": "image/png"}, + }, nil + }) + + up := NewUploader(creator, srv.Client()) + att, err := up.Upload(context.Background(), path) + if err != nil { + t.Fatalf("Upload: %v", err) + } + + if gotBlob.Filename != "diagram.png" { + t.Errorf("blob filename = %q, want diagram.png", gotBlob.Filename) + } + if gotBlob.ContentType != "image/png" { + t.Errorf("blob content type = %q, want image/png", gotBlob.ContentType) + } + if gotBlob.ByteSize != int64(len(data)) { + t.Errorf("blob byte size = %d, want %d", gotBlob.ByteSize, len(data)) + } + if gotBlob.Checksum != wantChecksum { + t.Errorf("blob checksum = %q, want %q", gotBlob.Checksum, wantChecksum) + } + if string(putBody) != string(data) { + t.Errorf("uploaded bytes = %q, want %q", putBody, data) + } + if putContentType != "image/png" { + t.Errorf("PUT content type = %q, want image/png", putContentType) + } + if att.SGID != "sgid-xyz" { + t.Errorf("attachment SGID = %q, want sgid-xyz", att.SGID) + } + if att.Filename != "diagram.png" { + t.Errorf("attachment filename = %q", att.Filename) + } +} + +func TestUploadPutFailure(t *testing.T) { + path := writeTempFile(t, "diagram.png", []byte("x")) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer srv.Close() + + creator := creatorFunc(func(ctx context.Context, blob Blob) (*DirectUpload, error) { + return &DirectUpload{URL: srv.URL, Headers: map[string]string{"Content-Type": "image/png"}}, nil + }) + + up := NewUploader(creator, srv.Client()) + if _, err := up.Upload(context.Background(), path); err == nil { + t.Fatal("expected error on non-2xx PUT") + } +} + +func TestUploadValidatesBeforeCreate(t *testing.T) { + called := false + creator := creatorFunc(func(ctx context.Context, blob Blob) (*DirectUpload, error) { + called = true + return &DirectUpload{}, nil + }) + up := NewUploader(creator, http.DefaultClient) + if _, err := up.Upload(context.Background(), filepath.Join(t.TempDir(), "missing.png")); err == nil { + t.Fatal("expected error for missing file") + } + if called { + t.Error("creator should not be called when validation fails") + } +} + +func TestMarkupEscaping(t *testing.T) { + att := &Attachment{ + Filename: `evil".png`, + ContentType: "image/png", + ByteSize: 42, + SGID: "sgid-1", + } + markup := att.Markup() + if strings.Contains(markup, `evil"`) { + t.Errorf("filename not escaped in markup: %s", markup) + } + if !strings.Contains(markup, "sgid-1") { + t.Errorf("markup missing sgid: %s", markup) + } + if !strings.Contains(markup, "action-text-attachment") { + t.Errorf("markup missing action-text-attachment element: %s", markup) + } +} + +func TestAppendMarkup(t *testing.T) { + atts := []*Attachment{ + {Filename: "a.png", ContentType: "image/png", ByteSize: 1, SGID: "s1"}, + {Filename: "b.pdf", ContentType: "application/pdf", ByteSize: 2, SGID: "s2"}, + } + + combined := AppendMarkup("Hello there", atts) + if !strings.HasPrefix(combined, "Hello there") { + t.Errorf("combined content should start with original message: %q", combined) + } + if !strings.Contains(combined, "s1") || !strings.Contains(combined, "s2") { + t.Errorf("combined content missing attachment sgids: %q", combined) + } + + empty := AppendMarkup("", atts) + if strings.HasPrefix(empty, "\n") { + t.Errorf("empty message should not start with newline: %q", empty) + } + + none := AppendMarkup("Hello", nil) + if none != "Hello" { + t.Errorf("no attachments should leave content unchanged, got %q", none) + } +} + +// creatorFunc adapts a function to the DirectUploadCreator interface. +type creatorFunc func(ctx context.Context, blob Blob) (*DirectUpload, error) + +func (f creatorFunc) CreateDirectUpload(ctx context.Context, blob Blob) (*DirectUpload, error) { + return f(ctx, blob) +} + +func md5Sum(b []byte) []byte { + sum := md5.Sum(b) + return sum[:] +} diff --git a/internal/cmd/compose.go b/internal/cmd/compose.go index 0add1f4..0e65360 100644 --- a/internal/cmd/compose.go +++ b/internal/cmd/compose.go @@ -1,24 +1,27 @@ package cmd import ( + "context" "fmt" "strconv" "strings" "github.com/spf13/cobra" + "github.com/basecamp/hey-cli/internal/attachments" "github.com/basecamp/hey-cli/internal/editor" "github.com/basecamp/hey-cli/internal/output" ) type composeCommand struct { - cmd *cobra.Command - to string - cc string - bcc string - subject string - message string - threadID string + cmd *cobra.Command + to string + cc string + bcc string + subject string + message string + threadID string + attachments []string } func newComposeCommand() *composeCommand { @@ -27,10 +30,11 @@ func newComposeCommand() *composeCommand { Use: "compose", Short: "Compose a new message", Annotations: map[string]string{ - "agent_notes": "Creates a new email. Requires --subject. Use --to (optionally with --cc/--bcc) for new threads or --thread-id for existing ones.", + "agent_notes": "Creates a new email. Requires --subject. Use --to (optionally with --cc/--bcc) for new threads or --thread-id for existing ones. Attach files with repeatable -a/--attachment; works for both new messages and --thread-id replies.", }, Example: ` hey compose --to alice@example.com --subject "Hello" -m "Hi there" hey compose --to alice@example.com --cc bob@example.com --bcc carol@example.org --subject "Hello" -m "Hi" + hey compose --to alice@example.com --subject "Q3 deck" -m "See attached" -a ~/reports/q3.pdf -a ~/charts/revenue.png hey compose --subject "Update" --thread-id 12345 -m "Thread reply" echo "Long message" | hey compose --to bob@example.com --subject "Report"`, RunE: composeCommand.run, @@ -42,6 +46,7 @@ func newComposeCommand() *composeCommand { composeCommand.cmd.Flags().StringVar(&composeCommand.subject, "subject", "", "Message subject (required)") composeCommand.cmd.Flags().StringVarP(&composeCommand.message, "message", "m", "", "Message body (or opens $EDITOR)") composeCommand.cmd.Flags().StringVar(&composeCommand.threadID, "thread-id", "", "Thread ID to post message to") + composeCommand.cmd.Flags().StringArrayVarP(&composeCommand.attachments, "attachment", "a", nil, "Path to a file to attach (repeatable)") return composeCommand } @@ -80,6 +85,11 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { ctx := cmd.Context() + message, err := c.attachFiles(ctx, message) + if err != nil { + return err + } + if c.threadID != "" { topicID, err := strconv.ParseInt(c.threadID, 10, 64) if err != nil { @@ -105,6 +115,36 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { return writeOK(nil, output.WithSummary("Message sent")) } +// attachFiles validates every attachment path, uploads each file through the +// Active Storage direct-upload flow, and appends the resulting markup to the +// message content. With no attachments it returns the content unchanged so +// existing behavior is preserved. +func (c *composeCommand) attachFiles(ctx context.Context, message string) (string, error) { + if len(c.attachments) == 0 { + return message, nil + } + + // Validate everything up front so a bad path never results in a partial + // upload or a sent message. + for _, path := range c.attachments { + if err := attachments.Validate(path); err != nil { + return "", err + } + } + + uploader := newAttachmentUploader() + uploaded := make([]*attachments.Attachment, 0, len(c.attachments)) + for _, path := range c.attachments { + att, err := uploader.Upload(ctx, path) + if err != nil { + return "", err + } + uploaded = append(uploaded, att) + } + + return attachments.AppendMarkup(message, uploaded), nil +} + func parseAddresses(s string) []string { if s == "" { return nil diff --git a/internal/cmd/compose_attachments.go b/internal/cmd/compose_attachments.go new file mode 100644 index 0000000..5c90398 --- /dev/null +++ b/internal/cmd/compose_attachments.go @@ -0,0 +1,68 @@ +package cmd + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + + "github.com/basecamp/hey-cli/internal/attachments" + "github.com/basecamp/hey-cli/internal/output" +) + +// directUploadsPath is HEY's Active Storage direct-upload endpoint. +const directUploadsPath = "/rails/active_storage/direct_uploads.json" + +// newAttachmentUploader builds an uploader backed by the SDK client. Blob bytes +// are PUT to a self-authenticating Active Storage URL, so the transfer uses a +// plain HTTP client with no HEY credentials. +func newAttachmentUploader() *attachments.Uploader { + return attachments.NewUploader(&sdkDirectUploadCreator{client: sdk}, http.DefaultClient) +} + +// sdkDirectUploadCreator creates Active Storage direct uploads through the HEY +// SDK client, keeping the HEY API call inside the SDK abstraction. +type sdkDirectUploadCreator struct { + client *hey.Client +} + +func (c *sdkDirectUploadCreator) CreateDirectUpload(ctx context.Context, blob attachments.Blob) (*attachments.DirectUpload, error) { + body := map[string]any{ + "blob": map[string]any{ + "filename": blob.Filename, + "byte_size": blob.ByteSize, + "checksum": blob.Checksum, + "content_type": blob.ContentType, + }, + } + resp, err := c.client.PostMutation(ctx, directUploadsPath, body) + if err != nil { + return nil, convertSDKError(err) + } + return parseDirectUpload(resp.Data) +} + +func parseDirectUpload(data []byte) (*attachments.DirectUpload, error) { + var payload struct { + SignedID string `json:"signed_id"` + AttachableSGID string `json:"attachable_sgid"` + DirectUpload struct { + URL string `json:"url"` + Headers map[string]string `json:"headers"` + } `json:"direct_upload"` + } + if err := json.Unmarshal(data, &payload); err != nil { + return nil, output.ErrAPI(0, fmt.Sprintf("could not parse direct upload response: %v", err)) + } + if payload.DirectUpload.URL == "" { + return nil, output.ErrAPI(0, "direct upload response missing upload URL") + } + return &attachments.DirectUpload{ + SignedID: payload.SignedID, + AttachableSGID: payload.AttachableSGID, + URL: payload.DirectUpload.URL, + Headers: payload.DirectUpload.Headers, + }, nil +} diff --git a/internal/cmd/compose_attachments_test.go b/internal/cmd/compose_attachments_test.go new file mode 100644 index 0000000..3a800c6 --- /dev/null +++ b/internal/cmd/compose_attachments_test.go @@ -0,0 +1,122 @@ +package cmd + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + + "github.com/basecamp/hey-cli/internal/attachments" + "github.com/basecamp/hey-cli/internal/output" +) + +type noopAuth struct{} + +func (noopAuth) Authenticate(context.Context, *http.Request) error { return nil } + +func TestParseDirectUpload(t *testing.T) { + body := []byte(`{ + "signed_id": "signed-abc", + "attachable_sgid": "sgid-xyz", + "direct_upload": { + "url": "https://example.com/upload/blob", + "headers": {"Content-Type": "image/png", "Content-MD5": "abc=="} + } + }`) + + upload, err := parseDirectUpload(body) + if err != nil { + t.Fatalf("parseDirectUpload: %v", err) + } + if upload.SignedID != "signed-abc" { + t.Errorf("SignedID = %q, want signed-abc", upload.SignedID) + } + if upload.AttachableSGID != "sgid-xyz" { + t.Errorf("AttachableSGID = %q, want sgid-xyz", upload.AttachableSGID) + } + if upload.URL != "https://example.com/upload/blob" { + t.Errorf("URL = %q", upload.URL) + } + if upload.Headers["Content-Type"] != "image/png" { + t.Errorf("Content-Type header = %q", upload.Headers["Content-Type"]) + } +} + +func TestParseDirectUploadMissingURL(t *testing.T) { + if _, err := parseDirectUpload([]byte(`{"signed_id":"x"}`)); err == nil { + t.Fatal("expected error when upload URL is missing") + } +} + +func TestSDKDirectUploadCreator(t *testing.T) { + var gotBody string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/rails/active_storage/direct_uploads.json" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + b, _ := io.ReadAll(r.Body) + gotBody = string(b) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"signed_id":"sig","attachable_sgid":"sgid","direct_upload":{"url":"` + srvURL(r) + `/up","headers":{"Content-Type":"application/pdf"}}}`)) + })) + defer srv.Close() + + client := hey.NewClient(&hey.Config{BaseURL: srv.URL, CacheEnabled: false}, nil, hey.WithAuthStrategy(noopAuth{})) + creator := &sdkDirectUploadCreator{client: client} + + upload, err := creator.CreateDirectUpload(context.Background(), attachments.Blob{ + Filename: "report.pdf", + ContentType: "application/pdf", + ByteSize: 1234, + Checksum: "deadbeef==", + }) + if err != nil { + t.Fatalf("CreateDirectUpload: %v", err) + } + if upload.SignedID != "sig" || upload.AttachableSGID != "sgid" { + t.Errorf("unexpected upload: %+v", upload) + } + for _, want := range []string{`"filename":"report.pdf"`, `"byte_size":1234`, `"checksum":"deadbeef=="`, `"content_type":"application/pdf"`} { + if !strings.Contains(gotBody, want) { + t.Errorf("request body missing %s; got %s", want, gotBody) + } + } +} + +func srvURL(r *http.Request) string { + return "http://" + r.Host +} + +func TestAttachFilesNoAttachments(t *testing.T) { + c := &composeCommand{} + got, err := c.attachFiles(context.Background(), "unchanged body") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "unchanged body" { + t.Errorf("content = %q, want unchanged", got) + } +} + +func TestAttachFilesMissingFileFailsBeforeSend(t *testing.T) { + c := &composeCommand{attachments: []string{filepath.Join(t.TempDir(), "ghost.png")}} + _, err := c.attachFiles(context.Background(), "body") + if err == nil { + t.Fatal("expected error for missing attachment") + } + if e := output.AsError(err); e.Code != "usage" { + t.Errorf("error code = %q, want usage", e.Code) + } +} + +func TestAttachFilesDirectoryFails(t *testing.T) { + c := &composeCommand{attachments: []string{t.TempDir()}} + if _, err := c.attachFiles(context.Background(), "body"); err == nil { + t.Fatal("expected error for directory attachment") + } +} From dbe51c90286fdf6dd00816ea796b05474543383c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Victor?= Date: Mon, 29 Jun 2026 17:00:59 -0300 Subject: [PATCH 2/2] test: cover compose attachment edge cases --- internal/attachments/attachments.go | 77 +++++++----- internal/attachments/attachments_test.go | 33 +++++- internal/cmd/compose.go | 13 ++- internal/cmd/compose_attachments.go | 9 +- internal/cmd/compose_attachments_test.go | 142 +++++++++++++++++++++++ 5 files changed, 241 insertions(+), 33 deletions(-) diff --git a/internal/attachments/attachments.go b/internal/attachments/attachments.go index 62631f9..93fc6c7 100644 --- a/internal/attachments/attachments.go +++ b/internal/attachments/attachments.go @@ -13,7 +13,6 @@ package attachments import ( - "bytes" "context" "crypto/md5" //nolint:gosec // Active Storage requires an MD5 checksum, not for security "encoding/base64" @@ -127,24 +126,21 @@ func (u *Uploader) Upload(ctx context.Context, path string) (*Attachment, error) return nil, err } - data, err := os.ReadFile(path) //nolint:gosec // path is user-provided by design + blob, file, err := blobForFile(path) if err != nil { - return nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) - } - - blob := Blob{ - Filename: filepath.Base(path), - ContentType: detectContentType(path, data), - ByteSize: int64(len(data)), - Checksum: checksum(data), + return nil, err } + defer func() { _ = file.Close() }() upload, err := u.creator.CreateDirectUpload(ctx, blob) if err != nil { return nil, err } + if upload.AttachableSGID == "" { + return nil, apierr.ErrAPI(0, "direct upload response missing attachable SGID") + } - if err := u.put(ctx, upload, data); err != nil { + if err := u.put(ctx, upload, file); err != nil { return nil, err } @@ -153,12 +149,12 @@ func (u *Uploader) Upload(ctx context.Context, path string) (*Attachment, error) ContentType: blob.ContentType, ByteSize: blob.ByteSize, SignedID: upload.SignedID, - SGID: attachableSGID(upload), + SGID: upload.AttachableSGID, }, nil } -func (u *Uploader) put(ctx context.Context, upload *DirectUpload, data []byte) error { - req, err := http.NewRequestWithContext(ctx, http.MethodPut, upload.URL, bytes.NewReader(data)) +func (u *Uploader) put(ctx context.Context, upload *DirectUpload, body io.Reader) error { + req, err := http.NewRequestWithContext(ctx, http.MethodPut, upload.URL, body) if err != nil { return apierr.ErrAPI(0, fmt.Sprintf("could not build upload request: %v", err)) } @@ -191,26 +187,53 @@ func (a *Attachment) Markup() string { ) } -// attachableSGID prefers the attachable signed global ID, falling back to the -// blob's signed ID when the server omits it. -func attachableSGID(upload *DirectUpload) string { - if upload.AttachableSGID != "" { - return upload.AttachableSGID +func blobForFile(path string) (Blob, *os.File, error) { + info, err := os.Stat(path) + if err != nil { + return Blob{}, nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + + file, err := os.Open(path) //nolint:gosec // path is user-provided by design + if err != nil { + return Blob{}, nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) } - return upload.SignedID + + sample := make([]byte, 512) + n, err := file.Read(sample) + if err != nil && err != io.EOF { + _ = file.Close() + return Blob{}, nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + _ = file.Close() + return Blob{}, nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + + h := md5.New() //nolint:gosec // Active Storage requires an MD5 checksum, not for security + if _, err := io.Copy(h, file); err != nil { + _ = file.Close() + return Blob{}, nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + _ = file.Close() + return Blob{}, nil, apierr.ErrUsage(fmt.Sprintf("cannot read attachment %s: %v", path, err)) + } + + blob := Blob{ + Filename: filepath.Base(path), + ContentType: detectContentType(path, sample[:n]), + ByteSize: info.Size(), + Checksum: base64.StdEncoding.EncodeToString(h.Sum(nil)), + } + return blob, file, nil } -func detectContentType(path string, data []byte) string { +func detectContentType(path string, sample []byte) string { if ct := mime.TypeByExtension(filepath.Ext(path)); ct != "" { return ct } - if ct := http.DetectContentType(data); ct != "application/octet-stream" { + if ct := http.DetectContentType(sample); ct != "application/octet-stream" { return ct } return "application/octet-stream" } - -func checksum(data []byte) string { - sum := md5.Sum(data) //nolint:gosec // Active Storage requires an MD5 checksum, not for security - return base64.StdEncoding.EncodeToString(sum[:]) -} diff --git a/internal/attachments/attachments_test.go b/internal/attachments/attachments_test.go index 4997d0d..b48dec9 100644 --- a/internal/attachments/attachments_test.go +++ b/internal/attachments/attachments_test.go @@ -118,7 +118,12 @@ func TestUploadPutFailure(t *testing.T) { defer srv.Close() creator := creatorFunc(func(ctx context.Context, blob Blob) (*DirectUpload, error) { - return &DirectUpload{URL: srv.URL, Headers: map[string]string{"Content-Type": "image/png"}}, nil + return &DirectUpload{ + SignedID: "signed-abc", + AttachableSGID: "sgid-xyz", + URL: srv.URL, + Headers: map[string]string{"Content-Type": "image/png"}, + }, nil }) up := NewUploader(creator, srv.Client()) @@ -127,6 +132,32 @@ func TestUploadPutFailure(t *testing.T) { } } +func TestUploadMissingAttachableSGIDFailsBeforePut(t *testing.T) { + path := writeTempFile(t, "diagram.png", []byte("x")) + putCalled := false + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + putCalled = true + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + + creator := creatorFunc(func(ctx context.Context, blob Blob) (*DirectUpload, error) { + return &DirectUpload{ + SignedID: "signed-abc", + URL: srv.URL, + }, nil + }) + + up := NewUploader(creator, srv.Client()) + if _, err := up.Upload(context.Background(), path); err == nil { + t.Fatal("expected error when attachable SGID is missing") + } + if putCalled { + t.Fatal("blob PUT should not be attempted without an attachable SGID") + } +} + func TestUploadValidatesBeforeCreate(t *testing.T) { called := false creator := creatorFunc(func(ctx context.Context, blob Blob) (*DirectUpload, error) { diff --git a/internal/cmd/compose.go b/internal/cmd/compose.go index 0e65360..c29d522 100644 --- a/internal/cmd/compose.go +++ b/internal/cmd/compose.go @@ -85,16 +85,21 @@ func (c *composeCommand) run(cmd *cobra.Command, args []string) error { ctx := cmd.Context() + var topicID int64 + if c.threadID != "" { + var err error + topicID, err = strconv.ParseInt(c.threadID, 10, 64) + if err != nil { + return output.ErrUsage(fmt.Sprintf("invalid thread ID: %s", c.threadID)) + } + } + message, err := c.attachFiles(ctx, message) if err != nil { return err } if c.threadID != "" { - topicID, err := strconv.ParseInt(c.threadID, 10, 64) - if err != nil { - return output.ErrUsage(fmt.Sprintf("invalid thread ID: %s", c.threadID)) - } if err := sdk.Messages().CreateTopicMessage(ctx, topicID, message); err != nil { return convertSDKError(err) } diff --git a/internal/cmd/compose_attachments.go b/internal/cmd/compose_attachments.go index 5c90398..8366b95 100644 --- a/internal/cmd/compose_attachments.go +++ b/internal/cmd/compose_attachments.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "time" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -19,7 +20,7 @@ const directUploadsPath = "/rails/active_storage/direct_uploads.json" // are PUT to a self-authenticating Active Storage URL, so the transfer uses a // plain HTTP client with no HEY credentials. func newAttachmentUploader() *attachments.Uploader { - return attachments.NewUploader(&sdkDirectUploadCreator{client: sdk}, http.DefaultClient) + return attachments.NewUploader(&sdkDirectUploadCreator{client: sdk}, &http.Client{Timeout: 30 * time.Second}) } // sdkDirectUploadCreator creates Active Storage direct uploads through the HEY @@ -56,6 +57,12 @@ func parseDirectUpload(data []byte) (*attachments.DirectUpload, error) { if err := json.Unmarshal(data, &payload); err != nil { return nil, output.ErrAPI(0, fmt.Sprintf("could not parse direct upload response: %v", err)) } + if payload.SignedID == "" { + return nil, output.ErrAPI(0, "direct upload response missing signed ID") + } + if payload.AttachableSGID == "" { + return nil, output.ErrAPI(0, "direct upload response missing attachable SGID") + } if payload.DirectUpload.URL == "" { return nil, output.ErrAPI(0, "direct upload response missing upload URL") } diff --git a/internal/cmd/compose_attachments_test.go b/internal/cmd/compose_attachments_test.go index 3a800c6..39c31fd 100644 --- a/internal/cmd/compose_attachments_test.go +++ b/internal/cmd/compose_attachments_test.go @@ -2,9 +2,11 @@ package cmd import ( "context" + "encoding/json" "io" "net/http" "net/http/httptest" + "os" "path/filepath" "strings" "testing" @@ -53,6 +55,20 @@ func TestParseDirectUploadMissingURL(t *testing.T) { } } +func TestParseDirectUploadMissingSignedID(t *testing.T) { + body := []byte(`{"attachable_sgid":"sgid","direct_upload":{"url":"https://example.com/upload"}}`) + if _, err := parseDirectUpload(body); err == nil { + t.Fatal("expected error when signed_id is missing") + } +} + +func TestParseDirectUploadMissingAttachableSGID(t *testing.T) { + body := []byte(`{"signed_id":"signed","direct_upload":{"url":"https://example.com/upload"}}`) + if _, err := parseDirectUpload(body); err == nil { + t.Fatal("expected error when attachable_sgid is missing") + } +} + func TestSDKDirectUploadCreator(t *testing.T) { var gotBody string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -120,3 +136,129 @@ func TestAttachFilesDirectoryFails(t *testing.T) { t.Fatal("expected error for directory attachment") } } + +func TestAttachFilesMultipleAttachmentsUploadsAllInOrder(t *testing.T) { + first := writeCmdTempFile(t, "first.pdf", []byte("first pdf")) + second := writeCmdTempFile(t, "second.png", []byte("second png")) + + var created []string + var uploaded []string + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + defer srv.Close() + + mux.HandleFunc("/rails/active_storage/direct_uploads.json", func(w http.ResponseWriter, r *http.Request) { + var body struct { + Blob struct { + Filename string `json:"filename"` + } `json:"blob"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode direct upload body: %v", err) + } + created = append(created, body.Blob.Filename) + idx := len(created) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"signed_id":"signed-` + body.Blob.Filename + `","attachable_sgid":"sgid-` + body.Blob.Filename + `","direct_upload":{"url":"` + srv.URL + `/upload/` + string(rune('0'+idx)) + `","headers":{"Content-Type":"application/octet-stream"}}}`)) + }) + mux.HandleFunc("/upload/1", func(w http.ResponseWriter, r *http.Request) { + uploaded = append(uploaded, "first") + w.WriteHeader(http.StatusNoContent) + }) + mux.HandleFunc("/upload/2", func(w http.ResponseWriter, r *http.Request) { + uploaded = append(uploaded, "second") + w.WriteHeader(http.StatusNoContent) + }) + + oldSDK := sdk + sdk = hey.NewClient(&hey.Config{BaseURL: srv.URL, CacheEnabled: false}, nil, hey.WithAuthStrategy(noopAuth{})) + t.Cleanup(func() { sdk = oldSDK }) + + c := &composeCommand{attachments: []string{first, second}} + content, err := c.attachFiles(context.Background(), "body") + if err != nil { + t.Fatalf("attachFiles: %v", err) + } + + if strings.Join(created, ",") != "first.pdf,second.png" { + t.Fatalf("created direct uploads = %v, want first.pdf then second.png", created) + } + if strings.Join(uploaded, ",") != "first,second" { + t.Fatalf("uploaded blobs = %v, want first then second", uploaded) + } + firstIndex := strings.Index(content, "sgid-first.pdf") + secondIndex := strings.Index(content, "sgid-second.png") + if firstIndex < 0 || secondIndex < 0 || firstIndex > secondIndex { + t.Fatalf("attachment markup order wrong: %q", content) + } +} + +func TestAttachFilesInvalidAmongMultipleDoesNotUpload(t *testing.T) { + valid := writeCmdTempFile(t, "valid.pdf", []byte("valid")) + missing := filepath.Join(t.TempDir(), "missing.pdf") + + called := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + oldSDK := sdk + sdk = hey.NewClient(&hey.Config{BaseURL: srv.URL, CacheEnabled: false}, nil, hey.WithAuthStrategy(noopAuth{})) + t.Cleanup(func() { sdk = oldSDK }) + + c := &composeCommand{attachments: []string{valid, missing}} + if _, err := c.attachFiles(context.Background(), "body"); err == nil { + t.Fatal("expected missing file error") + } + if called { + t.Fatal("server should not be called when one attachment path is invalid") + } +} + +func TestComposeInvalidThreadIDDoesNotUploadAttachments(t *testing.T) { + attachment := writeCmdTempFile(t, "valid.pdf", []byte("valid")) + called := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/rails/active_storage/direct_uploads.json" { + called = true + } + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + _, err := runComposeForAttachmentTest(t, srv, "--subject", "Update", "--thread-id", "not-a-number", "-m", "body", "-a", attachment) + if err == nil { + t.Fatal("expected invalid thread ID error") + } + if called { + t.Fatal("direct upload should not be called when thread ID is invalid") + } +} + +func runComposeForAttachmentTest(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() + root.SetArgs(append([]string{"compose", "--json", "--base-url", server.URL}, args...)) + + err := root.Execute() + return output.Response{}, err +} + +func writeCmdTempFile(t *testing.T, name string, data []byte) string { + t.Helper() + path := filepath.Join(t.TempDir(), name) + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("write temp file: %v", err) + } + return path +}