fix: address all 12 PR review findings

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()

cli/mailbox_test.go

@@ -101,24 +101,21 @@ 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)
 	cmd.SetErr(buf)
 	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)
 	}
 }
 

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
 }

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)

internal/jmap/client.go

@@ -9,6 +9,8 @@ import (
 	"net/http"
 	"strings"
 	"sync"
+
+	"github.com/samber/oops"
 )
 
 // Client is a JMAP protocol client.
@@ -65,15 +67,15 @@ 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)
 	req.Header.Set("Accept", "application/json")
 
 	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)

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)
+		})
+	}
+}

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)
 }

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.

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)
 	}
 }