From 2c1dab1bb2d4650bfe9b9c9ffde883059e8a5d16 Mon Sep 17 00:00:00 2001 From: thibaud-leclere Date: Fri, 10 Apr 2026 12:14:55 +0200 Subject: [PATCH] fix: align mcp mailbox and limit contracts --- internal/imapclient/backend.go | 10 ++--- internal/mcpserver/server.go | 17 +++----- internal/mcpserver/server_test.go | 71 ++++++++++++++++++++++++++++--- 3 files changed, 78 insertions(+), 20 deletions(-) diff --git a/internal/imapclient/backend.go b/internal/imapclient/backend.go index 4d0a39f..5d14554 100644 --- a/internal/imapclient/backend.go +++ b/internal/imapclient/backend.go @@ -19,8 +19,8 @@ import ( ) const ( - defaultListMessagesLimit = 20 - maxListMessagesLimit = 50 + DefaultListMessagesLimit = 20 + MaxListMessagesLimit = 50 imapImplicitTLSPort = "993" ) @@ -276,10 +276,10 @@ func imapAddress(host string) string { func clampListLimit(limit int) int { if limit <= 0 { - return defaultListMessagesLimit + return DefaultListMessagesLimit } - if limit > maxListMessagesLimit { - return maxListMessagesLimit + if limit > MaxListMessagesLimit { + return MaxListMessagesLimit } return limit } diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index 19618fb..dbb9718 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -15,11 +15,6 @@ import ( var ErrCredentialsNotConfigured = errors.New("credentials not configured; run `email-mcp setup`") -const ( - defaultListMessagesLimit = 20 - maxListMessagesLimit = 50 -) - type MailService interface { ListMailboxes(context.Context, secretstore.Credential) ([]imapclient.Mailbox, error) ListMessages(context.Context, secretstore.Credential, string, int) ([]imapclient.MessageSummary, error) @@ -97,12 +92,13 @@ func (s Server) Tools() []Tool { "mailbox": map[string]any{ "type": "string", "minLength": 1, + "pattern": "\\S", }, "limit": map[string]any{ "type": "integer", - "default": defaultListMessagesLimit, + "default": imapclient.DefaultListMessagesLimit, "minimum": 1, - "maximum": maxListMessagesLimit, + "maximum": imapclient.MaxListMessagesLimit, }, }, "required": []string{"mailbox"}, @@ -118,6 +114,7 @@ func (s Server) Tools() []Tool { "mailbox": map[string]any{ "type": "string", "minLength": 1, + "pattern": "\\S", }, "uid": map[string]any{ "type": "integer", @@ -303,10 +300,10 @@ func validateMailbox(mailbox string) (string, error) { func normalizeListMessagesLimit(limit *int) (int, error) { if limit == nil { - return defaultListMessagesLimit, nil + return imapclient.DefaultListMessagesLimit, nil } - if *limit < 1 || *limit > maxListMessagesLimit { - return 0, fmt.Errorf("limit must be between 1 and %d", maxListMessagesLimit) + if *limit < 1 || *limit > imapclient.MaxListMessagesLimit { + return 0, fmt.Errorf("limit must be between 1 and %d", imapclient.MaxListMessagesLimit) } return *limit, nil } diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index 8768044..b7a2328 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -273,14 +273,21 @@ func TestServerToolsAdvertiseValidatedArgumentContracts(t *testing.T) { if !ok { t.Fatalf("expected limit schema, got %#v", listProps["limit"]) } - if got := limitSchema["default"]; got != float64(defaultListMessagesLimit) && got != defaultListMessagesLimit { - t.Fatalf("expected limit default %d, got %#v", defaultListMessagesLimit, got) + mailboxSchema, ok := listProps["mailbox"].(map[string]any) + if !ok { + t.Fatalf("expected mailbox schema, got %#v", listProps["mailbox"]) + } + if got := mailboxSchema["pattern"]; got != "\\S" { + t.Fatalf("expected mailbox pattern %q, got %#v", "\\S", got) + } + if got := limitSchema["default"]; got != float64(imapclient.DefaultListMessagesLimit) && got != imapclient.DefaultListMessagesLimit { + t.Fatalf("expected limit default %d, got %#v", imapclient.DefaultListMessagesLimit, got) } if got := limitSchema["minimum"]; got != float64(1) && got != 1 { t.Fatalf("expected limit minimum 1, got %#v", got) } - if got := limitSchema["maximum"]; got != float64(maxListMessagesLimit) && got != maxListMessagesLimit { - t.Fatalf("expected limit maximum %d, got %#v", maxListMessagesLimit, got) + if got := limitSchema["maximum"]; got != float64(imapclient.MaxListMessagesLimit) && got != imapclient.MaxListMessagesLimit { + t.Fatalf("expected limit maximum %d, got %#v", imapclient.MaxListMessagesLimit, got) } getMessage := tools[2] @@ -291,6 +298,13 @@ func TestServerToolsAdvertiseValidatedArgumentContracts(t *testing.T) { if !ok { t.Fatalf("expected get_message properties map, got %#v", getMessage.InputSchema["properties"]) } + getMailboxSchema, ok := getProps["mailbox"].(map[string]any) + if !ok { + t.Fatalf("expected get_message mailbox schema, got %#v", getProps["mailbox"]) + } + if got := getMailboxSchema["pattern"]; got != "\\S" { + t.Fatalf("expected mailbox pattern %q, got %#v", "\\S", got) + } uidSchema, ok := getProps["uid"].(map[string]any) if !ok { t.Fatalf("expected uid schema, got %#v", getProps["uid"]) @@ -357,6 +371,53 @@ func TestRunnerRunReturnsValidationErrorsForInvalidRequests(t *testing.T) { } } +func TestRunnerRunRejectsWhitespaceOnlyMailboxValues(t *testing.T) { + store := &storeStub{ + credential: secretstore.Credential{ + Host: "imap.example.com", + Username: "alice", + Password: "secret", + }, + } + input := bytes.NewBufferString("{\"tool\":\"list_messages\",\"arguments\":{\"mailbox\":\" \"}}\n") + output := &bytes.Buffer{} + runner := NewRunner(New(store, serviceStub{ + listMailboxes: func(context.Context, secretstore.Credential) ([]imapclient.Mailbox, error) { + t.Fatal("ListMailboxes should not be called") + return nil, nil + }, + listMessages: func(context.Context, secretstore.Credential, string, int) ([]imapclient.MessageSummary, error) { + t.Fatal("ListMessages should not be called") + return nil, nil + }, + getMessage: func(context.Context, secretstore.Credential, string, uint32) (imapclient.Message, error) { + t.Fatal("GetMessage should not be called") + return imapclient.Message{}, nil + }, + }), input, output, &bytes.Buffer{}) + + if err := runner.Run(context.Background()); err != nil { + t.Fatalf("Run returned error: %v", err) + } + + decoder := json.NewDecoder(output) + if err := decoder.Decode(&struct { + Tools []Tool `json:"tools"` + }{}); err != nil { + t.Fatalf("failed to decode manifest: %v", err) + } + + var response struct { + Error string `json:"error"` + } + if err := decoder.Decode(&response); err != nil { + t.Fatalf("failed to decode error response: %v", err) + } + if response.Error != "mailbox is required" { + t.Fatalf("unexpected error: %#v", response) + } +} + func TestRunnerRunAppliesDefaultLimitWhenOmitted(t *testing.T) { store := &storeStub{ credential: secretstore.Credential{ @@ -373,7 +434,7 @@ func TestRunnerRunAppliesDefaultLimitWhenOmitted(t *testing.T) { return nil, nil }, listMessages: func(_ context.Context, cred secretstore.Credential, mailbox string, limit int) ([]imapclient.MessageSummary, error) { - if cred.Host != "imap.example.com" || mailbox != "INBOX" || limit != defaultListMessagesLimit { + if cred.Host != "imap.example.com" || mailbox != "INBOX" || limit != imapclient.DefaultListMessagesLimit { t.Fatalf("unexpected call: cred=%#v mailbox=%q limit=%d", cred, mailbox, limit) } return []imapclient.MessageSummary{{UID: 42, Subject: "hello", From: "alice@example.com"}}, nil