diff --git a/cli/mailbox.go b/cli/mailbox.go index a92f3a4..9ffdcaf 100644 --- a/cli/mailbox.go +++ b/cli/mailbox.go @@ -140,8 +140,7 @@ func newMailboxDeleteCommand() *cobra.Command { id := args[0] if !force { - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Are you sure you want to delete mailbox %s? Use --force to confirm.\n", id) - return nil + return fmt.Errorf("use --force to confirm deletion of mailbox %s", id) } client, err := createClient() diff --git a/cli/mailbox_test.go b/cli/mailbox_test.go index b474db2..a86180b 100644 --- a/cli/mailbox_test.go +++ b/cli/mailbox_test.go @@ -101,7 +101,7 @@ func TestMailboxDelete_RequiresIDArg(t *testing.T) { } } -func TestMailboxDelete_NoForceShowsWarning(t *testing.T) { +func TestMailboxDelete_NoForceReturnsError(t *testing.T) { cmd := NewRootCommand() buf := new(bytes.Buffer) cmd.SetOut(buf) @@ -109,16 +109,13 @@ func TestMailboxDelete_NoForceShowsWarning(t *testing.T) { cmd.SetArgs([]string{"mailbox", "delete", "mb-123"}) err := cmd.Execute() - if err != nil { - t.Fatalf("mailbox delete without --force should not error: %v", err) + if err == nil { + t.Fatal("mailbox delete without --force should return an error") } - output := buf.String() - if !strings.Contains(output, "Are you sure") { - t.Errorf("expected confirmation warning, got: %q", output) - } - if !strings.Contains(output, "--force") { - t.Errorf("expected --force hint, got: %q", output) + errMsg := err.Error() + if !strings.Contains(errMsg, "--force") { + t.Errorf("expected error to mention --force, got: %q", errMsg) } } diff --git a/cli/vacation.go b/cli/vacation.go index 886b4b7..cc1f807 100644 --- a/cli/vacation.go +++ b/cli/vacation.go @@ -112,6 +112,7 @@ func newVacationSetCommand() *cobra.Command { cmd.Flags().StringVar(&toDate, "to", "", "end date (RFC3339)") cmd.Flags().StringVar(&subject, "subject", "", "auto-reply subject") cmd.Flags().StringVar(&textBody, "body", "", "auto-reply body text") + cmd.MarkFlagsMutuallyExclusive("enable", "disable") return cmd } diff --git a/cli/vacation_test.go b/cli/vacation_test.go index 81885d3..7c8b0eb 100644 --- a/cli/vacation_test.go +++ b/cli/vacation_test.go @@ -22,6 +22,23 @@ func TestVacationSet_NoFlags_ReturnsError(t *testing.T) { } } +func TestVacationSet_EnableDisableMutuallyExclusive(t *testing.T) { + cmd := NewRootCommand() + buf := new(bytes.Buffer) + cmd.SetOut(buf) + cmd.SetErr(buf) + cmd.SetArgs([]string{"vacation", "set", "--enable", "--disable"}) + + err := cmd.Execute() + if err == nil { + t.Fatal("vacation set with both --enable and --disable should return an error") + } + // Cobra's mutually exclusive error says "if any flags in the group [enable disable] are set none of the others can be" + if !strings.Contains(err.Error(), "if any flags in the group") { + t.Errorf("expected mutually exclusive flags error, got: %v", err) + } +} + func TestVacationHelp_ShowsSubcommands(t *testing.T) { cmd := NewRootCommand() buf := new(bytes.Buffer) diff --git a/internal/jmap/client.go b/internal/jmap/client.go index 41e09f2..6f989c1 100644 --- a/internal/jmap/client.go +++ b/internal/jmap/client.go @@ -9,6 +9,8 @@ import ( "net/http" "strings" "sync" + + "github.com/samber/oops" ) // Client is a JMAP protocol client. @@ -65,7 +67,7 @@ func NewClient(endpoint, accessToken string, opts ...ClientOption) *Client { func (c *Client) Authenticate(ctx context.Context) (*Session, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.endpoint, http.NoBody) if err != nil { - return nil, fmt.Errorf("creating request: %w", err) + return nil, oops.Wrapf(err, "creating request") } req.Header.Set("Authorization", "Bearer "+c.accessToken) @@ -73,7 +75,7 @@ func (c *Client) Authenticate(ctx context.Context) (*Session, error) { resp, err := c.httpClient.Do(req) if err != nil { - return nil, fmt.Errorf("executing request: %w", err) + return nil, oops.Wrapf(err, "executing request") } defer func() { _ = resp.Body.Close() }() @@ -84,7 +86,7 @@ func (c *Client) Authenticate(ctx context.Context) (*Session, error) { var session Session if err := json.NewDecoder(resp.Body).Decode(&session); err != nil { - return nil, fmt.Errorf("decoding session: %w", err) + return nil, oops.Wrapf(err, "decoding session") } // Cache the session @@ -113,17 +115,17 @@ func (c *Client) Session(ctx context.Context) (*Session, error) { func (c *Client) Call(ctx context.Context, request *Request) (*Response, error) { session, err := c.Session(ctx) if err != nil { - return nil, fmt.Errorf("getting session: %w", err) + return nil, oops.Wrapf(err, "getting session") } reqBody, err := json.Marshal(request) if err != nil { - return nil, fmt.Errorf("marshaling request: %w", err) + return nil, oops.Wrapf(err, "marshaling request") } httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, session.APIURL, bytes.NewReader(reqBody)) if err != nil { - return nil, fmt.Errorf("creating request: %w", err) + return nil, oops.Wrapf(err, "creating request") } httpReq.Header.Set("Authorization", "Bearer "+c.accessToken) @@ -132,7 +134,7 @@ func (c *Client) Call(ctx context.Context, request *Request) (*Response, error) resp, err := c.httpClient.Do(httpReq) if err != nil { - return nil, fmt.Errorf("executing request: %w", err) + return nil, oops.Wrapf(err, "executing request") } defer func() { _ = resp.Body.Close() }() @@ -143,7 +145,7 @@ func (c *Client) Call(ctx context.Context, request *Request) (*Response, error) var response Response if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { - return nil, fmt.Errorf("decoding response: %w", err) + return nil, oops.Wrapf(err, "decoding response") } return &response, nil @@ -153,7 +155,7 @@ func (c *Client) Call(ctx context.Context, request *Request) (*Response, error) func (c *Client) DownloadBlob(ctx context.Context, accountID, blobID string) (io.ReadCloser, error) { session, err := c.Session(ctx) if err != nil { - return nil, fmt.Errorf("getting session: %w", err) + return nil, oops.Wrapf(err, "getting session") } url := session.DownloadURL @@ -164,13 +166,13 @@ func (c *Client) DownloadBlob(ctx context.Context, accountID, blobID string) (io req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) if err != nil { - return nil, fmt.Errorf("creating request: %w", err) + return nil, oops.Wrapf(err, "creating request") } req.Header.Set("Authorization", "Bearer "+c.accessToken) resp, err := c.httpClient.Do(req) if err != nil { - return nil, fmt.Errorf("downloading blob: %w", err) + return nil, oops.Wrapf(err, "downloading blob") } if resp.StatusCode != http.StatusOK { body, _ := io.ReadAll(resp.Body) diff --git a/internal/jmap/client_test.go b/internal/jmap/client_test.go index 79aafee..ba9caa7 100644 --- a/internal/jmap/client_test.go +++ b/internal/jmap/client_test.go @@ -3,12 +3,14 @@ package jmap import ( "context" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" "strings" "testing" + "github.com/samber/oops" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -457,3 +459,62 @@ func TestClient_DownloadBlob_HTTPError(t *testing.T) { assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) } } + +func TestClient_ErrorsAreOopsErrors(t *testing.T) { + tests := []struct { + name string + fn func(t *testing.T) error + }{ + { + name: "Authenticate decode error returns oops error", + fn: func(t *testing.T) error { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{invalid json`)) + })) + defer server.Close() + + client := NewClient(server.URL, "token") + _, err := client.Authenticate(context.Background()) + return err + }, + }, + { + name: "Authenticate network error returns oops error", + fn: func(t *testing.T) error { + t.Helper() + client := NewClient("http://127.0.0.1:0", "token") + _, err := client.Authenticate(context.Background()) + return err + }, + }, + { + name: "Call session error returns oops error", + fn: func(t *testing.T) error { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer server.Close() + + client := NewClient(server.URL, "bad-token") + req := NewRequest() + req.Invoke("Test/method", map[string]any{}) + _, err := client.Call(context.Background(), req) + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.fn(t) + require.Error(t, err) + + var oopsErr oops.OopsError + assert.True(t, errors.As(err, &oopsErr), "expected oops.OopsError, got %T: %v", err, err) + }) + } +} diff --git a/internal/jmap/response.go b/internal/jmap/response.go index 5aba48b..44f173b 100644 --- a/internal/jmap/response.go +++ b/internal/jmap/response.go @@ -3,6 +3,8 @@ package jmap import ( "encoding/json" "fmt" + + "github.com/samber/oops" ) // Response represents a JMAP API response. @@ -90,5 +92,5 @@ func (r *Response) GetResult(callID string) (*MethodResult, error) { return &r.MethodResponses[i], nil } } - return nil, fmt.Errorf("no result for call ID %q", callID) + return nil, oops.Errorf("no result for call ID %q", callID) } diff --git a/mcp/resources.go b/mcp/resources.go index 5893f9d..7388577 100644 --- a/mcp/resources.go +++ b/mcp/resources.go @@ -251,24 +251,14 @@ func (r *ResourceRegistry) handleMail(ctx context.Context, params map[string]str // handleContacts returns the contact list. func (r *ResourceRegistry) handleContacts(_ context.Context, _ map[string]string) (*ResourceContent, error) { - // Contacts require CardDAV client which may not be configured via JMAP - // Return informative message if not available - return &ResourceContent{ - URI: "fastmail://contacts", - MimeType: "text/plain", - Text: "Contacts access requires CardDAV configuration. Use the contacts_list tool instead.", - }, nil + // Contacts require CardDAV client which is not configured via JMAP + return nil, oops.Errorf("contacts access requires CardDAV configuration") } // handleCalendarToday returns today's calendar events. func (r *ResourceRegistry) handleCalendarToday(_ context.Context, _ map[string]string) (*ResourceContent, error) { - // Calendar service requires DAV client which may not be configured - // Return informative message if not available - return &ResourceContent{ - URI: "fastmail://calendar/today", - MimeType: "text/plain", - Text: "Calendar access requires CalDAV configuration. No events available.", - }, nil + // Calendar service requires DAV client which is not configured + return nil, oops.Errorf("calendar access requires CalDAV configuration") } // handleMaskedEmails returns the masked email list. diff --git a/mcp/resources_test.go b/mcp/resources_test.go index 05020e1..45c4ca0 100644 --- a/mcp/resources_test.go +++ b/mcp/resources_test.go @@ -228,14 +228,30 @@ func TestResourceContent(t *testing.T) { func TestCalendarToday_NoConfig(t *testing.T) { registry := NewResourceRegistry(nil) - // Calendar should return informative message even without client - content, err := registry.Read(context.Background(), "fastmail://calendar/today") - if err != nil { - t.Fatalf("unexpected error: %v", err) + // Calendar should return an error when CalDAV is not configured, + // not a fake success with error text in the body + _, err := registry.Read(context.Background(), "fastmail://calendar/today") + if err == nil { + t.Fatal("expected error when CalDAV is not configured, got nil") } - if content.Text == "" { - t.Error("expected informative message about calendar config") + if !strings.Contains(err.Error(), "CalDAV") { + t.Errorf("expected error to mention CalDAV configuration, got: %v", err) + } +} + +func TestContacts_NoConfig(t *testing.T) { + registry := NewResourceRegistry(nil) + + // Contacts should return an error when CardDAV is not configured, + // not a fake success with error text in the body + _, err := registry.Read(context.Background(), "fastmail://contacts") + if err == nil { + t.Fatal("expected error when CardDAV is not configured, got nil") + } + + if !strings.Contains(err.Error(), "CardDAV") { + t.Errorf("expected error to mention CardDAV configuration, got: %v", err) } } diff --git a/mcp/tools_mail.go b/mcp/tools_mail.go index 894b2f6..d062c46 100644 --- a/mcp/tools_mail.go +++ b/mcp/tools_mail.go @@ -132,8 +132,14 @@ func registerMailTools(s *Server, cfg ToolsConfig) { func makeMailListHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - folder := getStringArg(args, "folder", "Inbox") - limit := getUint64Arg(args, "limit", 10) + folder, err := getStringArg(args, "folder", "Inbox") + if err != nil { + return nil, err + } + limit, err := getUint64Arg(args, "limit", 10) + if err != nil { + return nil, err + } emails, err := cfg.Client.Mail().List(ctx, folder, limit) if err != nil { @@ -146,7 +152,10 @@ func makeMailListHandler(cfg ToolsConfig) ToolHandler { func makeMailGetHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -162,11 +171,17 @@ func makeMailGetHandler(cfg ToolsConfig) ToolHandler { func makeMailSearchHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - query := getStringArg(args, "query", "") + query, err := getStringArg(args, "query", "") + if err != nil { + return nil, err + } if query == "" { return nil, oops.Errorf("query is required") } - limit := getUint64Arg(args, "limit", 10) + limit, err := getUint64Arg(args, "limit", 10) + if err != nil { + return nil, err + } emails, err := cfg.Client.Mail().Search(ctx, query, limit) if err != nil { @@ -179,23 +194,41 @@ func makeMailSearchHandler(cfg ToolsConfig) ToolHandler { func makeMailSendHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - to := getStringSliceArg(args, "to") + to, err := getStringSliceArg(args, "to") + if err != nil { + return nil, err + } if len(to) == 0 { return nil, oops.Errorf("to is required") } - subject := getStringArg(args, "subject", "") + subject, err := getStringArg(args, "subject", "") + if err != nil { + return nil, err + } if subject == "" { return nil, oops.Errorf("subject is required") } - body := getStringArg(args, "body", "") + body, err := getStringArg(args, "body", "") + if err != nil { + return nil, err + } if body == "" { return nil, oops.Errorf("body is required") } + cc, err := getStringSliceArg(args, "cc") + if err != nil { + return nil, err + } + bcc, err := getStringSliceArg(args, "bcc") + if err != nil { + return nil, err + } + opts := fastmail.SendOptions{ To: parseAddresses(to), - Cc: parseAddresses(getStringSliceArg(args, "cc")), - Bcc: parseAddresses(getStringSliceArg(args, "bcc")), + Cc: parseAddresses(cc), + Bcc: parseAddresses(bcc), Subject: subject, Body: body, } @@ -214,15 +247,24 @@ func makeMailSendHandler(cfg ToolsConfig) ToolHandler { func makeMailReplyHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - emailID := getStringArg(args, "email_id", "") + emailID, err := getStringArg(args, "email_id", "") + if err != nil { + return nil, err + } if emailID == "" { return nil, oops.Errorf("email_id is required") } - body := getStringArg(args, "body", "") + body, err := getStringArg(args, "body", "") + if err != nil { + return nil, err + } if body == "" { return nil, oops.Errorf("body is required") } - replyAll := getBoolArg(args, "reply_all", false) + replyAll, err := getBoolArg(args, "reply_all", false) + if err != nil { + return nil, err + } opts := fastmail.ReplyOptions{ EmailID: emailID, @@ -244,11 +286,17 @@ func makeMailReplyHandler(cfg ToolsConfig) ToolHandler { func makeMailMoveHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } - folder := getStringArg(args, "folder", "") + folder, err := getStringArg(args, "folder", "") + if err != nil { + return nil, err + } if folder == "" { return nil, oops.Errorf("folder is required") } @@ -267,22 +315,49 @@ func makeMailMoveHandler(cfg ToolsConfig) ToolHandler { func makeMailFlagHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } + read, err := getBoolArg(args, "read", false) + if err != nil { + return nil, err + } + unread, err := getBoolArg(args, "unread", false) + if err != nil { + return nil, err + } + if read && unread { + return nil, oops.Errorf("read and unread are mutually exclusive") + } + + flagged, err := getBoolArg(args, "flagged", false) + if err != nil { + return nil, err + } + unflagged, err := getBoolArg(args, "unflagged", false) + if err != nil { + return nil, err + } + if flagged && unflagged { + return nil, oops.Errorf("flagged and unflagged are mutually exclusive") + } + keywords := make(map[string]bool) - if getBoolArg(args, "read", false) { + if read { keywords["$seen"] = true } - if getBoolArg(args, "unread", false) { + if unread { keywords["$seen"] = false } - if getBoolArg(args, "flagged", false) { + if flagged { keywords["$flagged"] = true } - if getBoolArg(args, "unflagged", false) { + if unflagged { keywords["$flagged"] = false } @@ -303,7 +378,10 @@ func makeMailFlagHandler(cfg ToolsConfig) ToolHandler { func makeMailDeleteHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -332,7 +410,10 @@ func registerThreadTools(s *Server, cfg ToolsConfig) { func makeThreadGetHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -392,9 +473,17 @@ func makeMaskedEmailListHandler(cfg ToolsConfig) ToolHandler { func makeMaskedEmailCreateHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { + domain, err := getStringArg(args, "domain", "") + if err != nil { + return nil, err + } + description, err := getStringArg(args, "description", "") + if err != nil { + return nil, err + } opts := fastmail.CreateMaskedEmailOptions{ - ForDomain: getStringArg(args, "domain", ""), - Description: getStringArg(args, "description", ""), + ForDomain: domain, + Description: description, } email, err := cfg.Client.MaskedEmail().Create(ctx, opts) @@ -459,18 +548,42 @@ func makeVacationGetHandler(cfg ToolsConfig) ToolHandler { func makeVacationSetHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { + fromDate, err := getStringArg(args, "from_date", "") + if err != nil { + return nil, err + } + toDate, err := getStringArg(args, "to_date", "") + if err != nil { + return nil, err + } + subject, err := getStringArg(args, "subject", "") + if err != nil { + return nil, err + } + textBody, err := getStringArg(args, "text_body", "") + if err != nil { + return nil, err + } opts := fastmail.SetVacationOptions{ - FromDate: getStringArg(args, "from_date", ""), - ToDate: getStringArg(args, "to_date", ""), - Subject: getStringArg(args, "subject", ""), - TextBody: getStringArg(args, "text_body", ""), + FromDate: fromDate, + ToDate: toDate, + Subject: subject, + TextBody: textBody, } - if getBoolArg(args, "enable", false) { + enable, err := getBoolArg(args, "enable", false) + if err != nil { + return nil, err + } + if enable { t := true opts.IsEnabled = &t } - if getBoolArg(args, "disable", false) { + disable, err := getBoolArg(args, "disable", false) + if err != nil { + return nil, err + } + if disable { f := false opts.IsEnabled = &f } @@ -560,7 +673,10 @@ func makeContactsGetHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("contacts client not configured") } - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -585,15 +701,27 @@ func makeContactsCreateHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("contacts client not configured") } - name := getStringArg(args, "name", "") + name, err := getStringArg(args, "name", "") + if err != nil { + return nil, err + } if name == "" { return nil, oops.Errorf("name is required") } + email, err := getStringArg(args, "email", "") + if err != nil { + return nil, err + } + phone, err := getStringArg(args, "phone", "") + if err != nil { + return nil, err + } + contact := &fastmail.Contact{ Name: name, - Email: getStringArg(args, "email", ""), - Phone: getStringArg(args, "phone", ""), + Email: email, + Phone: phone, } if err := cfg.Contacts.Create(ctx, contact); err != nil { @@ -616,7 +744,10 @@ func makeContactsUpdateHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("contacts client not configured") } - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -628,20 +759,26 @@ func makeContactsUpdateHandler(cfg ToolsConfig) ToolHandler { } // Apply provided fields - if name, ok := args["name"]; ok { - if s, ok := name.(string); ok { - existing.Name = s + if _, ok := args["name"]; ok { + name, err := getStringArg(args, "name", "") + if err != nil { + return nil, err } + existing.Name = name } - if email, ok := args["email"]; ok { - if s, ok := email.(string); ok { - existing.Email = s + if _, ok := args["email"]; ok { + email, err := getStringArg(args, "email", "") + if err != nil { + return nil, err } + existing.Email = email } - if phone, ok := args["phone"]; ok { - if s, ok := phone.(string); ok { - existing.Phone = s + if _, ok := args["phone"]; ok { + phone, err := getStringArg(args, "phone", "") + if err != nil { + return nil, err } + existing.Phone = phone } if err := cfg.Contacts.Update(ctx, existing); err != nil { @@ -664,7 +801,10 @@ func makeContactsDeleteHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("contacts client not configured") } - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -746,11 +886,17 @@ func makeCalendarEventsHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("calendar not configured") } - startStr := getStringArg(args, "start", "") + startStr, err := getStringArg(args, "start", "") + if err != nil { + return nil, err + } if startStr == "" { return nil, oops.Errorf("start is required") } - endStr := getStringArg(args, "end", "") + endStr, err := getStringArg(args, "end", "") + if err != nil { + return nil, err + } if endStr == "" { return nil, oops.Errorf("end is required") } @@ -764,7 +910,10 @@ func makeCalendarEventsHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Wrapf(err, "parsing end time") } - calendarID := getStringArg(args, "calendar_id", "") + calendarID, err := getStringArg(args, "calendar_id", "") + if err != nil { + return nil, err + } events, err := cfg.Calendar.ListEventsFunc(ctx, calendarID, start, end) if err != nil { @@ -781,19 +930,31 @@ func makeCalendarCreateHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("calendar not configured") } - calendarID := getStringArg(args, "calendar_id", "") + calendarID, err := getStringArg(args, "calendar_id", "") + if err != nil { + return nil, err + } if calendarID == "" { return nil, oops.Errorf("calendar_id is required") } - summary := getStringArg(args, "summary", "") + summary, err := getStringArg(args, "summary", "") + if err != nil { + return nil, err + } if summary == "" { return nil, oops.Errorf("summary is required") } - startStr := getStringArg(args, "start", "") + startStr, err := getStringArg(args, "start", "") + if err != nil { + return nil, err + } if startStr == "" { return nil, oops.Errorf("start is required") } - endStr := getStringArg(args, "end", "") + endStr, err := getStringArg(args, "end", "") + if err != nil { + return nil, err + } if endStr == "" { return nil, oops.Errorf("end is required") } @@ -807,14 +968,27 @@ func makeCalendarCreateHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Wrapf(err, "parsing end time") } + description, err := getStringArg(args, "description", "") + if err != nil { + return nil, err + } + location, err := getStringArg(args, "location", "") + if err != nil { + return nil, err + } + allDay, err := getBoolArg(args, "all_day", false) + if err != nil { + return nil, err + } + event := &fastmail.Event{ CalendarID: calendarID, Summary: summary, - Description: getStringArg(args, "description", ""), - Location: getStringArg(args, "location", ""), + Description: description, + Location: location, Start: start, End: end, - AllDay: getBoolArg(args, "all_day", false), + AllDay: allDay, } if err := cfg.Calendar.CreateEventFunc(ctx, event); err != nil { @@ -838,7 +1012,10 @@ func makeCalendarGetHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("calendar not configured") } - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -858,7 +1035,10 @@ func makeCalendarUpdateHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("calendar not configured") } - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -870,23 +1050,33 @@ func makeCalendarUpdateHandler(cfg ToolsConfig) ToolHandler { } // Apply updates - if s := getStringArg(args, "summary", ""); s != "" { + if s, sErr := getStringArg(args, "summary", ""); sErr != nil { + return nil, sErr + } else if s != "" { event.Summary = s } - if s := getStringArg(args, "location", ""); s != "" { + if s, sErr := getStringArg(args, "location", ""); sErr != nil { + return nil, sErr + } else if s != "" { event.Location = s } - if s := getStringArg(args, "description", ""); s != "" { + if s, sErr := getStringArg(args, "description", ""); sErr != nil { + return nil, sErr + } else if s != "" { event.Description = s } - if s := getStringArg(args, "start", ""); s != "" { + if s, sErr := getStringArg(args, "start", ""); sErr != nil { + return nil, sErr + } else if s != "" { t, parseErr := time.Parse(time.RFC3339, s) if parseErr != nil { return nil, oops.Wrapf(parseErr, "invalid start time") } event.Start = t } - if s := getStringArg(args, "end", ""); s != "" { + if s, sErr := getStringArg(args, "end", ""); sErr != nil { + return nil, sErr + } else if s != "" { t, parseErr := time.Parse(time.RFC3339, s) if parseErr != nil { return nil, oops.Wrapf(parseErr, "invalid end time") @@ -908,7 +1098,10 @@ func makeCalendarDeleteHandler(cfg ToolsConfig) ToolHandler { return nil, oops.Errorf("calendar not configured") } - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } @@ -968,62 +1161,79 @@ func convertEventsToMCP(events []fastmail.Event) []*Event { // Helper functions -func getStringArg(args map[string]any, key, defaultValue string) string { - if v, ok := args[key]; ok { - if s, ok := v.(string); ok { - return s - } +func getStringArg(args map[string]any, key, defaultValue string) (string, error) { + v, ok := args[key] + if !ok { + return defaultValue, nil } - return defaultValue + s, ok := v.(string) + if !ok { + return "", oops.Errorf("%s must be a string, got %T", key, v) + } + return s, nil } -func getUint64Arg(args map[string]any, key string, defaultValue uint64) uint64 { - if v, ok := args[key]; ok { - switch n := v.(type) { - case float64: - if n >= 0 { - return uint64(n) - } - case int: - if n >= 0 { - return uint64(n) - } - case int64: - if n >= 0 { - return uint64(n) - } - case uint64: - return n - } +func getUint64Arg(args map[string]any, key string, defaultValue uint64) (uint64, error) { + v, ok := args[key] + if !ok { + return defaultValue, nil + } + switch n := v.(type) { + case float64: + if n < 0 { + return 0, oops.Errorf("%s must be non-negative, got %v", key, v) + } + return uint64(n), nil + case int: + if n < 0 { + return 0, oops.Errorf("%s must be non-negative, got %v", key, v) + } + return uint64(n), nil + case int64: + if n < 0 { + return 0, oops.Errorf("%s must be non-negative, got %v", key, v) + } + return uint64(n), nil + case uint64: + return n, nil + default: + return 0, oops.Errorf("%s must be a number, got %T", key, v) } - return defaultValue } -func getBoolArg(args map[string]any, key string, defaultValue bool) bool { - if v, ok := args[key]; ok { - if b, ok := v.(bool); ok { - return b - } +func getBoolArg(args map[string]any, key string, defaultValue bool) (bool, error) { + v, ok := args[key] + if !ok { + return defaultValue, nil } - return defaultValue + b, ok := v.(bool) + if !ok { + return false, oops.Errorf("%s must be a boolean, got %T", key, v) + } + return b, nil } -func getStringSliceArg(args map[string]any, key string) []string { - if v, ok := args[key]; ok { - switch arr := v.(type) { - case []string: - return arr - case []any: - result := make([]string, 0, len(arr)) - for _, item := range arr { - if s, ok := item.(string); ok { - result = append(result, s) - } +func getStringSliceArg(args map[string]any, key string) ([]string, error) { + v, ok := args[key] + if !ok { + return nil, nil + } + switch arr := v.(type) { + case []string: + return arr, nil + case []any: + result := make([]string, 0, len(arr)) + for i, item := range arr { + s, ok := item.(string) + if !ok { + return nil, oops.Errorf("%s must contain only strings, element %d is %T", key, i, item) } - return result + result = append(result, s) } + return result, nil + default: + return nil, oops.Errorf("%s must be an array, got %T", key, v) } - return nil } func parseAddresses(addrs []string) []fastmail.EmailAddress { diff --git a/mcp/tools_mail_test.go b/mcp/tools_mail_test.go index 223245c..d990dc8 100644 --- a/mcp/tools_mail_test.go +++ b/mcp/tools_mail_test.go @@ -6,6 +6,9 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/seanb4t/fastmail-cli/pkg/fastmail" ) @@ -456,35 +459,32 @@ func TestHelperFunctions(t *testing.T) { "name": "test", } - if got := getStringArg(args, "name", "default"); got != "test" { - t.Errorf("expected 'test', got %q", got) - } - if got := getStringArg(args, "missing", "default"); got != "default" { - t.Errorf("expected 'default', got %q", got) - } + got, err := getStringArg(args, "name", "default") + require.NoError(t, err) + assert.Equal(t, "test", got) + + got, err = getStringArg(args, "missing", "default") + require.NoError(t, err) + assert.Equal(t, "default", got) }) t.Run("getUint64Arg", func(t *testing.T) { args := map[string]any{ - "float": float64(42), - "int": int(42), - "int64": int64(42), - "uint64": uint64(42), - "invalid": "not a number", + "float": float64(42), + "int": int(42), + "int64": int64(42), + "uint64": uint64(42), } for _, key := range []string{"float", "int", "int64", "uint64"} { - if got := getUint64Arg(args, key, 0); got != 42 { - t.Errorf("getUint64Arg(%s): expected 42, got %d", key, got) - } + got, err := getUint64Arg(args, key, 0) + require.NoError(t, err) + assert.Equal(t, uint64(42), got, "getUint64Arg(%s)", key) } - if got := getUint64Arg(args, "invalid", 10); got != 10 { - t.Errorf("expected default 10, got %d", got) - } - if got := getUint64Arg(args, "missing", 10); got != 10 { - t.Errorf("expected default 10, got %d", got) - } + got, err := getUint64Arg(args, "missing", 10) + require.NoError(t, err) + assert.Equal(t, uint64(10), got) }) t.Run("getBoolArg", func(t *testing.T) { @@ -493,15 +493,17 @@ func TestHelperFunctions(t *testing.T) { "false": false, } - if got := getBoolArg(args, "true", false); !got { - t.Error("expected true") - } - if got := getBoolArg(args, "false", true); got { - t.Error("expected false") - } - if got := getBoolArg(args, "missing", true); !got { - t.Error("expected default true") - } + got, err := getBoolArg(args, "true", false) + require.NoError(t, err) + assert.True(t, got) + + got, err = getBoolArg(args, "false", true) + require.NoError(t, err) + assert.False(t, got) + + got, err = getBoolArg(args, "missing", true) + require.NoError(t, err) + assert.True(t, got) }) t.Run("getStringSliceArg", func(t *testing.T) { @@ -510,15 +512,133 @@ func TestHelperFunctions(t *testing.T) { "anys": []any{"c", "d"}, } - if got := getStringSliceArg(args, "strings"); len(got) != 2 || got[0] != "a" { - t.Errorf("expected [a b], got %v", got) - } - if got := getStringSliceArg(args, "anys"); len(got) != 2 || got[0] != "c" { - t.Errorf("expected [c d], got %v", got) - } - if got := getStringSliceArg(args, "missing"); got != nil { - t.Errorf("expected nil, got %v", got) - } + got, err := getStringSliceArg(args, "strings") + require.NoError(t, err) + assert.Equal(t, []string{"a", "b"}, got) + + got, err = getStringSliceArg(args, "anys") + require.NoError(t, err) + assert.Equal(t, []string{"c", "d"}, got) + + got, err = getStringSliceArg(args, "missing") + require.NoError(t, err) + assert.Nil(t, got) + }) +} + +func TestHelperFunctions_TypeMismatch(t *testing.T) { + t.Run("getStringArg returns error on wrong type", func(t *testing.T) { + args := map[string]any{"id": 12345} + val, err := getStringArg(args, "id", "") + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a string") + assert.Contains(t, err.Error(), "id") + assert.Empty(t, val) + }) + + t.Run("getStringArg returns default when key absent", func(t *testing.T) { + args := map[string]any{} + val, err := getStringArg(args, "id", "default") + require.NoError(t, err) + assert.Equal(t, "default", val) + }) + + t.Run("getStringArg returns value when type correct", func(t *testing.T) { + args := map[string]any{"id": "abc"} + val, err := getStringArg(args, "id", "") + require.NoError(t, err) + assert.Equal(t, "abc", val) + }) + + t.Run("getUint64Arg returns error on wrong type", func(t *testing.T) { + args := map[string]any{"limit": "not-a-number"} + val, err := getUint64Arg(args, "limit", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a number") + assert.Contains(t, err.Error(), "limit") + assert.Equal(t, uint64(0), val) + }) + + t.Run("getUint64Arg returns default when key absent", func(t *testing.T) { + args := map[string]any{} + val, err := getUint64Arg(args, "limit", 10) + require.NoError(t, err) + assert.Equal(t, uint64(10), val) + }) + + t.Run("getUint64Arg returns value for float64", func(t *testing.T) { + args := map[string]any{"limit": float64(42)} + val, err := getUint64Arg(args, "limit", 0) + require.NoError(t, err) + assert.Equal(t, uint64(42), val) + }) + + t.Run("getUint64Arg returns error on negative float64", func(t *testing.T) { + args := map[string]any{"limit": float64(-1)} + val, err := getUint64Arg(args, "limit", 10) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be non-negative") + assert.Equal(t, uint64(0), val) + }) + + t.Run("getBoolArg returns error on wrong type", func(t *testing.T) { + args := map[string]any{"flag": "true"} + val, err := getBoolArg(args, "flag", false) + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a boolean") + assert.Contains(t, err.Error(), "flag") + assert.False(t, val) + }) + + t.Run("getBoolArg returns default when key absent", func(t *testing.T) { + args := map[string]any{} + val, err := getBoolArg(args, "flag", true) + require.NoError(t, err) + assert.True(t, val) + }) + + t.Run("getBoolArg returns value when type correct", func(t *testing.T) { + args := map[string]any{"flag": true} + val, err := getBoolArg(args, "flag", false) + require.NoError(t, err) + assert.True(t, val) + }) + + t.Run("getStringSliceArg returns error on non-string elements", func(t *testing.T) { + args := map[string]any{"tags": []any{"ok", 123, "also-ok"}} + val, err := getStringSliceArg(args, "tags") + require.Error(t, err) + assert.Contains(t, err.Error(), "must contain only strings") + assert.Nil(t, val) + }) + + t.Run("getStringSliceArg returns error on wrong type", func(t *testing.T) { + args := map[string]any{"tags": "not-a-slice"} + val, err := getStringSliceArg(args, "tags") + require.Error(t, err) + assert.Contains(t, err.Error(), "must be an array") + assert.Nil(t, val) + }) + + t.Run("getStringSliceArg returns nil when key absent", func(t *testing.T) { + args := map[string]any{} + val, err := getStringSliceArg(args, "tags") + require.NoError(t, err) + assert.Nil(t, val) + }) + + t.Run("getStringSliceArg returns value for []string", func(t *testing.T) { + args := map[string]any{"tags": []string{"a", "b"}} + val, err := getStringSliceArg(args, "tags") + require.NoError(t, err) + assert.Equal(t, []string{"a", "b"}, val) + }) + + t.Run("getStringSliceArg returns value for []any with strings", func(t *testing.T) { + args := map[string]any{"tags": []any{"c", "d"}} + val, err := getStringSliceArg(args, "tags") + require.NoError(t, err) + assert.Equal(t, []string{"c", "d"}, val) }) } @@ -637,6 +757,53 @@ func TestRegisterResourcesWithCalendar(t *testing.T) { } } +func TestMailFlagHandler_MutualExclusivity(t *testing.T) { + handler := makeMailFlagHandler(ToolsConfig{}) + + t.Run("read and unread are mutually exclusive", func(t *testing.T) { + _, err := handler(t.Context(), map[string]any{ + "id": "msg-1", + "read": true, + "unread": true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "mutually exclusive") + }) + + t.Run("flagged and unflagged are mutually exclusive", func(t *testing.T) { + _, err := handler(t.Context(), map[string]any{ + "id": "msg-1", + "flagged": true, + "unflagged": true, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "mutually exclusive") + }) + + t.Run("read alone passes validation", func(t *testing.T) { + // With nil client, handler will panic past validation when calling SetKeywords. + // A panic (not an error) confirms validation was passed successfully. + assert.Panics(t, func() { + _, _ = handler(t.Context(), map[string]any{ + "id": "msg-1", + "read": true, + }) + }) + }) + + t.Run("read and flagged together pass validation", func(t *testing.T) { + // With nil client, handler will panic past validation when calling SetKeywords. + // A panic (not an error) confirms validation was passed successfully. + assert.Panics(t, func() { + _, _ = handler(t.Context(), map[string]any{ + "id": "msg-1", + "read": true, + "flagged": true, + }) + }) + }) +} + func TestParseAddresses(t *testing.T) { addrs := []string{"alice@example.com", "bob@example.com"} result := parseAddresses(addrs) diff --git a/mcp/tools_mailbox.go b/mcp/tools_mailbox.go index 15d756b..95780e1 100644 --- a/mcp/tools_mailbox.go +++ b/mcp/tools_mailbox.go @@ -65,12 +65,18 @@ func makeMailboxListHandler(cfg ToolsConfig) ToolHandler { func makeMailboxCreateHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - name := getStringArg(args, "name", "") + name, err := getStringArg(args, "name", "") + if err != nil { + return nil, err + } if name == "" { return nil, oops.Errorf("name is required") } - parentID := getStringArg(args, "parent_id", "") + parentID, err := getStringArg(args, "parent_id", "") + if err != nil { + return nil, err + } mb, err := cfg.Client.Mailbox().Create(ctx, name, parentID) if err != nil { @@ -87,12 +93,18 @@ func makeMailboxCreateHandler(cfg ToolsConfig) ToolHandler { func makeMailboxRenameHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } - name := getStringArg(args, "name", "") + name, err := getStringArg(args, "name", "") + if err != nil { + return nil, err + } if name == "" { return nil, oops.Errorf("name is required") } @@ -111,7 +123,10 @@ func makeMailboxRenameHandler(cfg ToolsConfig) ToolHandler { func makeMailboxDeleteHandler(cfg ToolsConfig) ToolHandler { return func(ctx context.Context, args map[string]any) (any, error) { - id := getStringArg(args, "id", "") + id, err := getStringArg(args, "id", "") + if err != nil { + return nil, err + } if id == "" { return nil, oops.Errorf("id is required") } diff --git a/pkg/fastmail/identity_service.go b/pkg/fastmail/identity_service.go index 80095b1..c7fba49 100644 --- a/pkg/fastmail/identity_service.go +++ b/pkg/fastmail/identity_service.go @@ -31,7 +31,7 @@ type IdentityService struct { func (s *IdentityService) List(ctx context.Context) ([]Identity, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "listing identities") } getBuilder := jmap.NewIdentityGet(accountID) diff --git a/pkg/fastmail/mail.go b/pkg/fastmail/mail.go index 2fd6421..66ed7e6 100644 --- a/pkg/fastmail/mail.go +++ b/pkg/fastmail/mail.go @@ -2,9 +2,9 @@ package fastmail import ( "context" + "errors" "fmt" "io" - "os" "slices" "strings" "time" @@ -14,6 +14,9 @@ import ( "github.com/seanb4t/fastmail-cli/internal/jmap" ) +// ErrMailboxNotFound is returned when a mailbox cannot be found by name or role. +var ErrMailboxNotFound = errors.New("mailbox not found") + // MailService provides email operations. type MailService struct { client *Client @@ -25,7 +28,7 @@ type MailService struct { func (s *MailService) List(ctx context.Context, folder string, limit uint64) ([]Email, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "listing emails") } mailboxID, err := s.resolveMailbox(ctx, accountID, folder) @@ -80,14 +83,14 @@ func (s *MailService) List(ctx context.Context, folder string, limit uint64) ([] return nil, oops.Wrapf(err, "decoding email response") } - return convertEmails(getResp.List), nil + return convertEmails(getResp.List) } // Get returns a single email by ID. func (s *MailService) Get(ctx context.Context, id string) (*Email, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "getting email") } getBuilder := jmap.NewEmailGet(accountID). @@ -123,8 +126,9 @@ func (s *MailService) Get(ctx context.Context, id string) (*Email, error) { return nil, oops.Errorf("email not found: %s", id) } - emails := convertEmails(getResp.List) - return &emails[0], nil + emails, parseErr := convertEmails(getResp.List) + // Parse warnings are non-fatal for single email get — propagate but still return + return &emails[0], parseErr } // GetFull returns a single email with full display properties including @@ -132,7 +136,7 @@ func (s *MailService) Get(ctx context.Context, id string) (*Email, error) { func (s *MailService) GetFull(ctx context.Context, id string) (*Email, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "getting full email") } getBuilder := jmap.NewEmailGet(accountID). @@ -172,15 +176,16 @@ func (s *MailService) GetFull(ctx context.Context, id string) (*Email, error) { return nil, oops.Errorf("email not found: %s", id) } - email := convertFullEmail(getResp.List[0]) - return &email, nil + email, parseErr := convertFullEmail(getResp.List[0]) + // Parse warnings are non-fatal — propagate but still return the email + return &email, parseErr } // GetRaw returns the raw RFC 5322 source of an email. func (s *MailService) GetRaw(ctx context.Context, id string) (io.ReadCloser, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "getting raw email") } getBuilder := jmap.NewEmailGet(accountID). @@ -233,20 +238,38 @@ func (s *MailService) Search(ctx context.Context, query string, limit uint64) ([ func (s *MailService) SearchWithFilter(ctx context.Context, filter map[string]any, limit uint64) ([]Email, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "searching emails") + } + + // Copy filter to avoid mutating caller's map + filterCopy := make(map[string]any, len(filter)) + for k, v := range filter { + filterCopy[k] = v + } + + // Deep-copy nested conditions if present + if conditions, ok := filterCopy["conditions"].([]map[string]any); ok { + copiedConds := make([]map[string]any, len(conditions)) + for i, cond := range conditions { + copiedConds[i] = make(map[string]any, len(cond)) + for k, v := range cond { + copiedConds[i][k] = v + } + } + filterCopy["conditions"] = copiedConds } // Resolve inMailbox folder name to ID if present - if inMailbox, ok := filter["inMailbox"].(string); ok { + if inMailbox, ok := filterCopy["inMailbox"].(string); ok { resolvedID, resolveErr := s.resolveMailbox(ctx, accountID, inMailbox) if resolveErr != nil { return nil, resolveErr } - filter["inMailbox"] = resolvedID + filterCopy["inMailbox"] = resolvedID } // Also check nested conditions for compound filters - if conditions, ok := filter["conditions"].([]map[string]any); ok { + if conditions, ok := filterCopy["conditions"].([]map[string]any); ok { for _, cond := range conditions { if inMailbox, ok := cond["inMailbox"].(string); ok { resolvedID, resolveErr := s.resolveMailbox(ctx, accountID, inMailbox) @@ -260,7 +283,7 @@ func (s *MailService) SearchWithFilter(ctx context.Context, filter map[string]an queryArgs := map[string]any{ "accountId": accountID, - "filter": filter, + "filter": filterCopy, "sort": []jmap.Comparator{ {Property: "receivedAt", IsAscending: false}, }, @@ -307,7 +330,7 @@ func (s *MailService) SearchWithFilter(ctx context.Context, filter map[string]an return nil, oops.Wrapf(err, "decoding email response") } - return convertEmails(getResp.List), nil + return convertEmails(getResp.List) } // Move moves an email to a different folder. @@ -315,13 +338,7 @@ func (s *MailService) SearchWithFilter(ctx context.Context, filter map[string]an func (s *MailService) Move(ctx context.Context, id string, folder string) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err - } - - // First get the current email to know its mailboxes - email, err := s.Get(ctx, id) - if err != nil { - return oops.Wrapf(err, "getting email") + return oops.Wrapf(err, "moving email") } targetMailboxID, err := s.resolveMailbox(ctx, accountID, folder) @@ -329,7 +346,7 @@ func (s *MailService) Move(ctx context.Context, id string, folder string) error return oops.Wrapf(err, "resolving folder %q", folder) } - // Build new mailboxIds map - remove all current, add target + // Setting mailboxIds replaces all current mailbox assignments newMailboxIDs := map[string]bool{ targetMailboxID: true, } @@ -372,9 +389,6 @@ func (s *MailService) Move(ctx context.Context, id string, folder string) error return oops.Errorf("failed to move email: %s", errInfo.Error()) } - // Suppress unused variable warning - email was used to validate existence - _ = email - return nil } @@ -382,7 +396,7 @@ func (s *MailService) Move(ctx context.Context, id string, folder string) error func (s *MailService) Delete(ctx context.Context, id string) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "deleting email") } // Get the email to check if it's already in Trash @@ -394,8 +408,12 @@ func (s *MailService) Delete(ctx context.Context, id string) error { // Find the Trash mailbox trashID, err := s.resolveMailbox(ctx, accountID, "Trash") if err != nil { - // If no Trash folder, permanently delete - return s.destroy(ctx, accountID, id) + if errors.Is(err, ErrMailboxNotFound) { + // No Trash folder exists, permanently delete + return s.destroy(ctx, accountID, id) + } + // Transient error (network, auth, server) — do NOT silently destroy + return oops.Wrapf(err, "looking up Trash mailbox") } // Check if already in Trash @@ -451,14 +469,18 @@ func (s *MailService) destroy(ctx context.Context, accountID, id string) error { func (s *MailService) SetKeywords(ctx context.Context, id string, keywords map[string]bool) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "setting email keywords") } // Build patch map for JMAP Email/set update - // JMAP uses "keywords/$seen" as key with value true/false + // Per RFC 8621 §4.1.1: keyword values MUST be true; to remove, set to null (nil) patch := make(map[string]any, len(keywords)) for keyword, value := range keywords { - patch["keywords/"+keyword] = value + if value { + patch["keywords/"+keyword] = true + } else { + patch["keywords/"+keyword] = nil // null removes the keyword per RFC 8621 + } } setBuilder := jmap.NewEmailSet(accountID).Update(id, patch) @@ -558,19 +580,23 @@ func (s *MailService) resolveMailbox(ctx context.Context, accountID, folder stri } } - return "", oops.Errorf("mailbox not found: %s", folder) + return "", oops.Wrapf(ErrMailboxNotFound, "%s", folder) } // convertEmails converts JMAP emails to domain emails. -func convertEmails(jmapEmails []jmap.Email) []Email { +// Returns all emails even if some have invalid timestamps; the error aggregates +// any timestamp parse failures so callers can log or propagate them. +func convertEmails(jmapEmails []jmap.Email) ([]Email, error) { emails := make([]Email, len(jmapEmails)) + var parseErrors []string for i, je := range jmapEmails { var receivedAt time.Time if je.ReceivedAt != "" { var parseErr error receivedAt, parseErr = time.Parse(time.RFC3339, je.ReceivedAt) if parseErr != nil { - fmt.Fprintf(os.Stderr, "warning: invalid date %q for email %s\n", je.ReceivedAt, je.ID) + parseErrors = append(parseErrors, fmt.Sprintf( + "email %s: invalid ReceivedAt %q: %v", je.ID, je.ReceivedAt, parseErr)) } } @@ -598,12 +624,18 @@ func convertEmails(jmapEmails []jmap.Email) []Email { MailboxIDs: mailboxIDs, } } - return emails + + var err error + if len(parseErrors) > 0 { + err = oops.Errorf("timestamp parse warnings: %s", strings.Join(parseErrors, "; ")) + } + return emails, err } // convertFullEmail converts a JMAP email with full properties to a domain Email. -func convertFullEmail(je jmap.Email) Email { - email := convertEmails([]jmap.Email{je})[0] +func convertFullEmail(je jmap.Email) (Email, error) { + emails, parseErr := convertEmails([]jmap.Email{je}) + email := emails[0] if len(je.From) > 0 { email.From = EmailAddress{Name: je.From[0].Name, Email: je.From[0].Email} @@ -622,7 +654,7 @@ func convertFullEmail(je jmap.Email) Email { email.Attachments = convertJMAPAttachments(je.Attachments) - return email + return email, parseErr } // convertJMAPAddressToDomain converts JMAP email addresses to domain addresses. @@ -664,7 +696,7 @@ type SendOptions struct { func (s *MailService) Send(ctx context.Context, opts SendOptions) (string, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return "", err + return "", oops.Wrapf(err, "sending email") } // Get the default identity for sending @@ -697,7 +729,7 @@ type ReplyOptions struct { func (s *MailService) Reply(ctx context.Context, opts ReplyOptions) (string, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return "", err + return "", oops.Wrapf(err, "replying to email") } // Get the default identity diff --git a/pkg/fastmail/mail_test.go b/pkg/fastmail/mail_test.go index 2d40ed4..dd37b72 100644 --- a/pkg/fastmail/mail_test.go +++ b/pkg/fastmail/mail_test.go @@ -686,6 +686,95 @@ func TestMailService_SearchWithFilter_ResolvesInMailboxInConditions(t *testing.T assert.Empty(t, emails) } +func TestMailService_SearchWithFilter_DoesNotMutateCallerFilter(t *testing.T) { + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req map[string]any + err := json.NewDecoder(r.Body).Decode(&req) + require.NoError(t, err) + + methodCalls := req["methodCalls"].([]any) + firstCall := methodCalls[0].([]any) + methodName := firstCall[0].(string) + + w.Header().Set("Content-Type", "application/json") + + if methodName == testMethodMailboxGet { + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Mailbox/get", { + "accountId": "acc1", + "state": "m1", + "list": [ + {"id": "mb-inbox", "name": "Inbox", "role": "inbox"}, + {"id": "mb-drafts", "name": "Drafts", "role": "drafts"} + ], + "notFound": [] + }, "0"] + ] + }`)) + return + } + + if methodName == "Email/query" { + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/query", { + "accountId": "acc1", + "queryState": "q1", + "canCalculateChanges": true, + "position": 0, + "ids": [], + "total": 0 + }, "0"], + ["Email/get", { + "accountId": "acc1", + "state": "e1", + "list": [], + "notFound": [] + }, "1"] + ] + }`)) + return + } + })) + defer apiServer.Close() + + sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(sessionResponse(apiServer.URL))) + })) + defer sessionServer.Close() + + client := NewClient(sessionServer.URL, "test-token") + ctx := context.Background() + + t.Run("top-level inMailbox is not mutated", func(t *testing.T) { + filter := map[string]any{ + "inMailbox": "Inbox", + "from": "alice@example.com", + } + _, err := client.Mail().SearchWithFilter(ctx, filter, 10) + require.NoError(t, err) + assert.Equal(t, "Inbox", filter["inMailbox"], "caller's filter map should not be mutated") + }) + + t.Run("nested conditions inMailbox is not mutated", func(t *testing.T) { + conditions := []map[string]any{ + {"inMailbox": "Drafts"}, + {"from": "bob@example.com"}, + } + filter := map[string]any{ + "operator": "AND", + "conditions": conditions, + } + _, err := client.Mail().SearchWithFilter(ctx, filter, 10) + require.NoError(t, err) + assert.Equal(t, "Drafts", conditions[0]["inMailbox"], "caller's nested condition should not be mutated") + }) +} + func TestMailService_Move(t *testing.T) { apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req map[string]any @@ -774,6 +863,107 @@ func TestMailService_Move(t *testing.T) { require.NoError(t, err) } +func TestMailService_Move_NoRedundantGet(t *testing.T) { + // Track all JMAP method names across requests to verify Move does NOT + // make a redundant Email/get call before Email/set. + var allMethods []string + + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + + var req map[string]any + err = json.Unmarshal(body, &req) + require.NoError(t, err) + + methodCalls := req["methodCalls"].([]any) + firstCall := methodCalls[0].([]any) + methodName := firstCall[0].(string) + + // Record every method call in the request + for _, mc := range methodCalls { + call := mc.([]any) + allMethods = append(allMethods, call[0].(string)) + } + + w.Header().Set("Content-Type", "application/json") + + switch methodName { + case testMethodMailboxGet: + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Mailbox/get", { + "accountId": "acc1", + "state": "m1", + "list": [ + {"id": "mb-inbox", "name": "Inbox", "role": "inbox"}, + {"id": "mb-archive", "name": "Archive", "role": "archive"} + ], + "notFound": [] + }, "0"] + ] + }`)) + case testMethodEmailSet: + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/set", { + "accountId": "acc1", + "oldState": "e1", + "newState": "e2", + "updated": {"email-123": null} + }, "0"] + ] + }`)) + case testMethodEmailGet: + // This handler exists so the test doesn't crash if the redundant + // Get call is still present, but we assert below that it should + // never be reached. + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/get", { + "accountId": "acc1", + "state": "e1", + "list": [ + { + "id": "email-123", + "threadId": "t1", + "subject": "Test", + "preview": "...", + "receivedAt": "2024-01-15T10:30:00Z", + "size": 100, + "keywords": {}, + "mailboxIds": {"mb-inbox": true} + } + ], + "notFound": [] + }, "0"] + ] + }`)) + } + })) + defer apiServer.Close() + + sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(sessionResponse(apiServer.URL))) + })) + defer sessionServer.Close() + + client := NewClient(sessionServer.URL, "test-token") + ctx := context.Background() + + err := client.Mail().Move(ctx, "email-123", "Archive") + require.NoError(t, err) + + // Move should only issue Mailbox/get (to resolve folder) + Email/set. + // It must NOT make a redundant Email/get call to validate existence. + assert.Equal(t, []string{testMethodMailboxGet, testMethodEmailSet}, allMethods, + "Move should only call Mailbox/get and Email/set, not Email/get") +} + func TestMailService_Delete_MovesToTrash(t *testing.T) { apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req map[string]any @@ -945,6 +1135,169 @@ func TestMailService_Delete_PermanentlyDestroysFromTrash(t *testing.T) { require.NoError(t, err) } +func TestMailService_Delete_TransientErrorNotDestroy(t *testing.T) { + // When resolveMailbox fails for a transient reason (e.g., server error), + // Delete must propagate the error — NOT fall through to permanent deletion. + callCount := 0 + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req map[string]any + err := json.NewDecoder(r.Body).Decode(&req) + require.NoError(t, err) + + methodCalls := req["methodCalls"].([]any) + firstCall := methodCalls[0].([]any) + methodName := firstCall[0].(string) + + w.Header().Set("Content-Type", "application/json") + + switch methodName { + case testMethodEmailGet: + // Return a valid email so Get() succeeds + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/get", { + "accountId": "acc1", + "state": "e1", + "list": [ + { + "id": "email-123", + "threadId": "t1", + "subject": "Important", + "preview": "...", + "receivedAt": "2024-01-15T10:30:00Z", + "size": 100, + "keywords": {}, + "mailboxIds": {"mb-inbox": true} + } + ], + "notFound": [] + }, "0"] + ] + }`)) + case testMethodMailboxGet: + // Simulate a transient server error (500) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`Internal Server Error`)) + case testMethodEmailSet: + callCount++ + // Should NOT be reached — if it is, destroy was called + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/set", { + "accountId": "acc1", + "oldState": "e1", + "newState": "e2", + "destroyed": ["email-123"] + }, "0"] + ] + }`)) + } + })) + defer apiServer.Close() + + sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(sessionResponse(apiServer.URL))) + })) + defer sessionServer.Close() + + client := NewClient(sessionServer.URL, "test-token") + ctx := context.Background() + + err := client.Mail().Delete(ctx, "email-123") + require.Error(t, err, "Delete should propagate transient error, not silently destroy") + assert.Equal(t, 0, callCount, "Email/set (destroy) should not be called on transient error") +} + +func TestMailService_Delete_MailboxNotFound_Destroys(t *testing.T) { + // When the Trash mailbox genuinely does not exist (ErrMailboxNotFound), + // Delete should fall through to permanent destroy. + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req map[string]any + err := json.NewDecoder(r.Body).Decode(&req) + require.NoError(t, err) + + methodCalls := req["methodCalls"].([]any) + firstCall := methodCalls[0].([]any) + methodName := firstCall[0].(string) + + w.Header().Set("Content-Type", "application/json") + + switch methodName { + case testMethodEmailGet: + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/get", { + "accountId": "acc1", + "state": "e1", + "list": [ + { + "id": "email-456", + "threadId": "t1", + "subject": "No Trash Here", + "preview": "...", + "receivedAt": "2024-01-15T10:30:00Z", + "size": 100, + "keywords": {}, + "mailboxIds": {"mb-inbox": true} + } + ], + "notFound": [] + }, "0"] + ] + }`)) + case testMethodMailboxGet: + // Return mailboxes WITHOUT a Trash folder + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Mailbox/get", { + "accountId": "acc1", + "state": "m1", + "list": [ + {"id": "mb-inbox", "name": "Inbox", "role": "inbox"} + ], + "notFound": [] + }, "0"] + ] + }`)) + case testMethodEmailSet: + args := firstCall[1].(map[string]any) + destroy, hasDestroy := args["destroy"].([]any) + assert.True(t, hasDestroy, "expected destroy operation") + assert.Contains(t, destroy, "email-456") + + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/set", { + "accountId": "acc1", + "oldState": "e1", + "newState": "e2", + "destroyed": ["email-456"] + }, "0"] + ] + }`)) + } + })) + defer apiServer.Close() + + sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(sessionResponse(apiServer.URL))) + })) + defer sessionServer.Close() + + client := NewClient(sessionServer.URL, "test-token") + ctx := context.Background() + + err := client.Mail().Delete(ctx, "email-456") + require.NoError(t, err, "Delete should permanently destroy when Trash mailbox not found") +} + func TestClient_Connect(t *testing.T) { sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -1325,6 +1678,72 @@ func TestMailService_SetKeywords(t *testing.T) { require.NoError(t, err) } +func TestMailService_SetKeywords_RemovalUsesNull(t *testing.T) { + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + require.NoError(t, err) + + var req map[string]any + err = json.Unmarshal(body, &req) + require.NoError(t, err) + + methodCalls := req["methodCalls"].([]any) + firstCall := methodCalls[0].([]any) + methodName := firstCall[0].(string) + + w.Header().Set("Content-Type", "application/json") + require.Equal(t, testMethodEmailSet, methodName) + + // Verify the update payload + args := firstCall[1].(map[string]any) + update := args["update"].(map[string]any) + emailUpdate := update["email-kw-test"].(map[string]any) + + // Per RFC 8621 §4.1.1: setting a keyword uses true, + // removing a keyword uses null (not false) + assert.Equal(t, true, emailUpdate["keywords/$flagged"], + "setting a keyword should send true") + assert.Nil(t, emailUpdate["keywords/$seen"], + "removing a keyword should send null (nil), not false") + + // Also verify the raw JSON to be certain: $seen must be null, not false + rawPatch := json.RawMessage(body) + assert.Contains(t, string(rawPatch), `"keywords/$seen":null`, + "raw JSON should contain null for removed keyword, not false") + assert.NotContains(t, string(rawPatch), `"keywords/$seen":false`, + "raw JSON must not contain false for removed keyword") + + _, _ = w.Write([]byte(`{ + "sessionState": "s1", + "methodResponses": [ + ["Email/set", { + "accountId": "acc1", + "oldState": "e1", + "newState": "e2", + "updated": {"email-kw-test": null} + }, "0"] + ] + }`)) + })) + defer apiServer.Close() + + sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(sessionResponse(apiServer.URL))) + })) + defer sessionServer.Close() + + client := NewClient(sessionServer.URL, "test-token") + ctx := context.Background() + + keywords := map[string]bool{ + "$seen": false, // remove: should produce null in JSON + "$flagged": true, // set: should produce true in JSON + } + err := client.Mail().SetKeywords(ctx, "email-kw-test", keywords) + require.NoError(t, err) +} + func TestMailService_SetKeywords_NotUpdated(t *testing.T) { apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -1520,7 +1939,7 @@ func TestMailService_GetRaw_NoBlobID(t *testing.T) { } func TestConvertEmails_InvalidReceivedAt(t *testing.T) { - emails := convertEmails([]jmap.Email{ + emails, err := convertEmails([]jmap.Email{ { ID: "em1", Subject: "Valid date", @@ -1544,18 +1963,133 @@ func TestConvertEmails_InvalidReceivedAt(t *testing.T) { }, }) + // All emails should be returned even when some have parse errors require.Len(t, emails, 3) + // Error should be non-nil because em2 has invalid timestamp + require.Error(t, err) + assert.Contains(t, err.Error(), "em2") + assert.Contains(t, err.Error(), "not-a-date") + // Valid date should parse correctly assert.Equal(t, "em1", emails[0].ID) assert.False(t, emails[0].ReceivedAt.IsZero()) - // Invalid date should not panic, email should still be returned + // Invalid date should not panic, email should still be returned with zero time assert.Equal(t, "em2", emails[1].ID) assert.Equal(t, "Invalid date", emails[1].Subject) assert.True(t, emails[1].ReceivedAt.IsZero()) - // Empty date should result in zero time + // Empty date should result in zero time (not an error) assert.Equal(t, "em3", emails[2].ID) assert.True(t, emails[2].ReceivedAt.IsZero()) } + +func TestConvertEmails_AllValidTimestamps(t *testing.T) { + emails, err := convertEmails([]jmap.Email{ + { + ID: "em1", + Subject: "First", + ReceivedAt: "2024-01-15T10:30:00Z", + Keywords: map[string]bool{}, + MailboxIDs: map[string]bool{"mb1": true}, + }, + { + ID: "em2", + Subject: "Second", + ReceivedAt: "2024-02-20T14:00:00Z", + Keywords: map[string]bool{}, + MailboxIDs: map[string]bool{"mb1": true}, + }, + }) + + // When all timestamps are valid, error should be nil + require.NoError(t, err) + require.Len(t, emails, 2) + assert.False(t, emails[0].ReceivedAt.IsZero()) + assert.False(t, emails[1].ReceivedAt.IsZero()) +} + +func TestConvertEmails_EmptyInput(t *testing.T) { + emails, err := convertEmails([]jmap.Email{}) + require.NoError(t, err) + assert.Empty(t, emails) +} + +func TestGetAccountID_ErrorWrapping(t *testing.T) { + // Session server that always returns 500 to force getAccountID failure. + sessionServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "internal server error", http.StatusInternalServerError) + })) + defer sessionServer.Close() + + // Client with no cached accountID, so getAccountID must hit session endpoint. + client := NewClient(sessionServer.URL, "test-token") + ctx := context.Background() + + tests := []struct { + name string + call func() error + wantCtx string + }{ + { + name: "Mail.List wraps getAccountID error", + call: func() error { _, err := client.Mail().List(ctx, "Inbox", 10); return err }, + wantCtx: "listing emails", + }, + { + name: "Mail.Get wraps getAccountID error", + call: func() error { _, err := client.Mail().Get(ctx, "id1"); return err }, + wantCtx: "getting email", + }, + { + name: "Mail.Move wraps getAccountID error", + call: func() error { return client.Mail().Move(ctx, "id1", "Archive") }, + wantCtx: "moving email", + }, + { + name: "Mail.Delete wraps getAccountID error", + call: func() error { return client.Mail().Delete(ctx, "id1") }, + wantCtx: "deleting email", + }, + { + name: "Mail.Send wraps getAccountID error", + call: func() error { _, err := client.Mail().Send(ctx, SendOptions{}); return err }, + wantCtx: "sending email", + }, + { + name: "Mailbox.List wraps getAccountID error", + call: func() error { _, err := client.Mailbox().List(ctx); return err }, + wantCtx: "listing mailboxes", + }, + { + name: "Identity.List wraps getAccountID error", + call: func() error { _, err := client.Identity().List(ctx); return err }, + wantCtx: "listing identities", + }, + { + name: "Thread.Get wraps getAccountID error", + call: func() error { _, err := client.Thread().Get(ctx, "t1"); return err }, + wantCtx: "getting thread", + }, + { + name: "Vacation.Get wraps getAccountID error", + call: func() error { _, err := client.Vacation().Get(ctx); return err }, + wantCtx: "getting vacation response", + }, + { + name: "MaskedEmail.List wraps getAccountID error", + call: func() error { _, err := client.MaskedEmail().List(ctx); return err }, + wantCtx: "listing masked emails", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.call() + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantCtx, "error should contain operation context") + assert.Contains(t, err.Error(), "getting session", "error should contain underlying cause") + }) + } +} diff --git a/pkg/fastmail/mailbox_service.go b/pkg/fastmail/mailbox_service.go index d15d4e3..79327e2 100644 --- a/pkg/fastmail/mailbox_service.go +++ b/pkg/fastmail/mailbox_service.go @@ -17,7 +17,7 @@ type MailboxService struct { func (s *MailboxService) List(ctx context.Context) ([]Mailbox, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "listing mailboxes") } getBuilder := jmap.NewMailboxGet(accountID). @@ -51,7 +51,7 @@ func (s *MailboxService) List(ctx context.Context) ([]Mailbox, error) { func (s *MailboxService) Create(ctx context.Context, name, parentID string) (*Mailbox, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "creating mailbox") } data := map[string]any{"name": name} @@ -100,7 +100,7 @@ func (s *MailboxService) Create(ctx context.Context, name, parentID string) (*Ma func (s *MailboxService) Rename(ctx context.Context, id, newName string) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "renaming mailbox") } setBuilder := jmap.NewMailboxSet(accountID).Update(id, map[string]any{"name": newName}) @@ -136,7 +136,7 @@ func (s *MailboxService) Rename(ctx context.Context, id, newName string) error { func (s *MailboxService) Delete(ctx context.Context, id string) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "deleting mailbox") } setBuilder := jmap.NewMailboxSet(accountID).Destroy(id) diff --git a/pkg/fastmail/masked_email.go b/pkg/fastmail/masked_email.go index 5d88381..c04891b 100644 --- a/pkg/fastmail/masked_email.go +++ b/pkg/fastmail/masked_email.go @@ -49,7 +49,7 @@ type CreateMaskedEmailOptions struct { func (s *MaskedEmailService) List(ctx context.Context) ([]MaskedEmail, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "listing masked emails") } getBuilder := jmap.NewMaskedEmailGet(accountID) @@ -82,7 +82,7 @@ func (s *MaskedEmailService) List(ctx context.Context) ([]MaskedEmail, error) { func (s *MaskedEmailService) Create(ctx context.Context, opts CreateMaskedEmailOptions) (*MaskedEmail, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "creating masked email") } setBuilder := jmap.NewMaskedEmailSet(accountID). @@ -135,7 +135,7 @@ func (s *MaskedEmailService) Disable(ctx context.Context, id string) error { func (s *MaskedEmailService) updateState(ctx context.Context, id string, state jmap.MaskedEmailState) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "updating masked email state") } setBuilder := jmap.NewMaskedEmailSet(accountID). @@ -173,7 +173,7 @@ func (s *MaskedEmailService) updateState(ctx context.Context, id string, state j func (s *MaskedEmailService) Delete(ctx context.Context, id string) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "deleting masked email") } setBuilder := jmap.NewMaskedEmailSet(accountID).Destroy(id) diff --git a/pkg/fastmail/thread_service.go b/pkg/fastmail/thread_service.go index a76c34b..ef950b4 100644 --- a/pkg/fastmail/thread_service.go +++ b/pkg/fastmail/thread_service.go @@ -17,7 +17,7 @@ type ThreadService struct { func (s *ThreadService) Get(ctx context.Context, threadID string) ([]Email, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "getting thread") } // Step 1: Thread/get to get emailIds @@ -78,5 +78,5 @@ func (s *ThreadService) Get(ctx context.Context, threadID string) ([]Email, erro return nil, oops.Wrapf(err, "decoding email response") } - return convertEmails(getResp.List), nil + return convertEmails(getResp.List) } diff --git a/pkg/fastmail/vacation_service.go b/pkg/fastmail/vacation_service.go index 6bcc4ed..3920fd3 100644 --- a/pkg/fastmail/vacation_service.go +++ b/pkg/fastmail/vacation_service.go @@ -37,7 +37,7 @@ type VacationService struct { func (s *VacationService) Get(ctx context.Context) (*Vacation, error) { accountID, err := s.client.getAccountID(ctx) if err != nil { - return nil, err + return nil, oops.Wrapf(err, "getting vacation response") } getBuilder := jmap.NewVacationGet(accountID) @@ -81,7 +81,7 @@ func (s *VacationService) Get(ctx context.Context) (*Vacation, error) { func (s *VacationService) Set(ctx context.Context, opts SetVacationOptions) error { accountID, err := s.client.getAccountID(ctx) if err != nil { - return err + return oops.Wrapf(err, "setting vacation response") } patch := make(map[string]any)