diff --git a/CLAUDE.md b/CLAUDE.md index 77b0f24..521ba53 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,7 @@ Go 1.24+ · Hexagonal architecture (CLI → Port → Adapter) · Cobra CLI · ** | Hook enforcement | `.claude/HOOKS-CONFIG.md` | | Agent definitions | `.claude/agents/README.md` | -**Env vars:** `NYLAS_DISABLE_KEYRING`, `NYLAS_API_KEY`, `NYLAS_CLIENT_ID`, `NYLAS_GRANT_ID`, `NYLAS_API_BASE_URL` — see `docs/DEVELOPMENT.md` +**Env vars:** `NYLAS_DISABLE_KEYRING`, `NYLAS_API_KEY`, `NYLAS_CLIENT_ID`, `NYLAS_GRANT_ID`, `NYLAS_API_BASE_URL`, `NYLAS_API_TIMEOUT` (e.g. `120s`; or `nylas config set api.timeout`) — see `docs/DEVELOPMENT.md` **Credentials:** System keyring (service: `"nylas"`, keys: `client_id`, `api_key`, `client_secret`, `org_id`). Grant cache: `os.UserCacheDir()/nylas/grants.json`. Fallback: `~/.config/nylas/` with `NYLAS_DISABLE_KEYRING=true`. diff --git a/internal/adapters/nylas/client.go b/internal/adapters/nylas/client.go index 2e97f5e..e3b0cc0 100644 --- a/internal/adapters/nylas/client.go +++ b/internal/adapters/nylas/client.go @@ -9,12 +9,12 @@ import ( "fmt" "io" "net/http" + "slices" "strconv" "strings" "sync" "time" - "github.com/nylas/cli/internal/adapters/providers" "github.com/nylas/cli/internal/domain" "github.com/nylas/cli/internal/httputil" "github.com/nylas/cli/internal/ports" @@ -22,22 +22,6 @@ import ( "golang.org/x/time/rate" ) -func init() { - // Register Nylas provider with the global registry - providers.Register("nylas", func(config providers.ProviderConfig) (ports.NylasClient, error) { - client := NewHTTPClient() - - if config.BaseURL != "" { - client.SetBaseURL(config.BaseURL) - } else if config.Region != "" { - client.SetRegion(config.Region) - } - - client.SetCredentials(config.ClientID, config.ClientSecret, config.APIKey) - return client, nil - }) -} - const ( baseURLUS = domain.BaseURLUS baseURLEU = domain.BaseURLEU @@ -115,6 +99,16 @@ func (c *HTTPClient) ApplyConfig(cfg *domain.Config) { return } c.baseURL = cfg.ResolveBaseURL() + + // Apply the configured per-request API timeout. The shared DefaultClient is + // fixed at the default; when the install overrides it, give this client a + // matching http.Client so the transport-level cap tracks the request + // deadline instead of clipping (or out-living) it. + timeout := cfg.ResolveAPITimeout() + c.requestTimeout = timeout + if timeout != httputil.DefaultClientTimeout { + c.httpClient = httputil.NewClient(timeout) + } } // SetMaxRetries sets the maximum number of retries (for testing purposes). @@ -498,15 +492,7 @@ func (c *HTTPClient) doJSONRequestInternalWithRetry( } // Validate status code - statusOK := false - for _, status := range acceptedStatuses { - if resp.StatusCode == status { - statusOK = true - break - } - } - - if !statusOK { + if !slices.Contains(acceptedStatuses, resp.StatusCode) { defer func() { _ = resp.Body.Close() }() return nil, c.parseError(resp) } diff --git a/internal/adapters/nylas/security_test.go b/internal/adapters/nylas/security_test.go index 6276ffd..f2f923d 100644 --- a/internal/adapters/nylas/security_test.go +++ b/internal/adapters/nylas/security_test.go @@ -5,7 +5,9 @@ package nylas import ( "testing" + "time" + "github.com/nylas/cli/internal/domain" "github.com/nylas/cli/internal/httputil" ) @@ -116,6 +118,38 @@ func TestOTPExtractionSecurity(t *testing.T) { }) } +// TestHTTPClient_ApplyConfigTimeout verifies the per-request timeout is wired +// from config and that a non-default value also swaps in a matching http.Client +// so the transport-level cap tracks the request deadline. +func TestHTTPClient_ApplyConfigTimeout(t *testing.T) { + t.Run("default config keeps the shared client and default timeout", func(t *testing.T) { + t.Setenv("NYLAS_API_TIMEOUT", "") + client := NewHTTPClient() + client.ApplyConfig(&domain.Config{Region: "us"}) + if client.requestTimeout != domain.TimeoutAPI { + t.Errorf("requestTimeout = %v, want default %v", client.requestTimeout, domain.TimeoutAPI) + } + if client.httpClient != httputil.DefaultClient { + t.Error("expected the shared DefaultClient for the default timeout") + } + }) + + t.Run("config api.timeout sets request timeout and a matching client", func(t *testing.T) { + t.Setenv("NYLAS_API_TIMEOUT", "") + client := NewHTTPClient() + client.ApplyConfig(&domain.Config{Region: "us", API: &domain.APIConfig{Timeout: "300s"}}) + if want := 300 * time.Second; client.requestTimeout != want { + t.Errorf("requestTimeout = %v, want %v", client.requestTimeout, want) + } + if client.httpClient == httputil.DefaultClient { + t.Error("expected a dedicated client for a non-default timeout") + } + if want := 300 * time.Second; client.httpClient.Timeout != want { + t.Errorf("http client timeout = %v, want %v", client.httpClient.Timeout, want) + } + }) +} + // TestHTTPClientSecurity tests HTTP client security. func TestHTTPClientSecurity(t *testing.T) { t.Run("client_timeout_matches_server_ceiling", func(t *testing.T) { diff --git a/internal/adapters/providers/registry.go b/internal/adapters/providers/registry.go deleted file mode 100644 index 0c0e63a..0000000 --- a/internal/adapters/providers/registry.go +++ /dev/null @@ -1,38 +0,0 @@ -// Package providers implements a provider registry pattern for extensible multi-provider support. -package providers - -import ( - "sync" - - "github.com/nylas/cli/internal/ports" -) - -// ProviderFactory is a function that creates a new provider client -type ProviderFactory func(config ProviderConfig) (ports.NylasClient, error) - -// ProviderConfig contains configuration for initializing a provider -type ProviderConfig struct { - APIKey string - ClientID string - ClientSecret string - BaseURL string - Region string -} - -// Registry maintains a registry of provider factories -type Registry struct { - mu sync.RWMutex - factories map[string]ProviderFactory -} - -var defaultRegistry = &Registry{ - factories: make(map[string]ProviderFactory), -} - -// Register registers a provider factory with the default registry -// Providers should call this in their init() function -func Register(name string, factory ProviderFactory) { - defaultRegistry.mu.Lock() - defer defaultRegistry.mu.Unlock() - defaultRegistry.factories[name] = factory -} diff --git a/internal/cli/agent/rule.go b/internal/cli/agent/rule.go index 173dfd9..32a67c9 100644 --- a/internal/cli/agent/rule.go +++ b/internal/cli/agent/rule.go @@ -148,7 +148,9 @@ func detachRuleFromAgentWorkspaces(ctx context.Context, client interface { updatedWorkspaceIDs := make([]string, 0) for _, workspace := range workspaces { - if !stringSliceContains(workspace.RulesIDs, ruleID) { + if !slices.ContainsFunc(workspace.RulesIDs, func(id string) bool { + return strings.TrimSpace(id) == strings.TrimSpace(ruleID) + }) { continue } @@ -211,13 +213,3 @@ func rollbackWorkspaceRuleUpdates(ctx context.Context, client interface { } return nil } - -func stringSliceContains(items []string, value string) bool { - value = strings.TrimSpace(value) - for _, item := range items { - if strings.TrimSpace(item) == value { - return true - } - } - return false -} diff --git a/internal/cli/common/client.go b/internal/cli/common/client.go index 3cac302..d5f04aa 100644 --- a/internal/cli/common/client.go +++ b/internal/cli/common/client.go @@ -43,6 +43,10 @@ func GetNylasClient() (ports.NylasClient, error) { cfg = &domain.Config{Region: "us"} } + // Propagate the install's API timeout to CreateContext (the per-command + // deadline) so it matches the client this function builds. + SetAPITimeout(cfg.ResolveAPITimeout()) + // First, check environment variables (highest priority) apiKey := os.Getenv("NYLAS_API_KEY") clientID := os.Getenv("NYLAS_CLIENT_ID") diff --git a/internal/cli/common/context.go b/internal/cli/common/context.go index bc6b894..dbf18d7 100644 --- a/internal/cli/common/context.go +++ b/internal/cli/common/context.go @@ -2,15 +2,42 @@ package common import ( "context" + "sync/atomic" "time" "github.com/nylas/cli/internal/domain" ) -// CreateContext creates a context with the standard API timeout. +// apiTimeoutNanos holds the resolved per-request API timeout (0 = use the +// domain.TimeoutAPI default). It is set once at client construction +// (GetNylasClient) from the install's config/env and read by CreateContext, +// which runs on every command. atomic because spinner/background goroutines +// may also create contexts. +var apiTimeoutNanos atomic.Int64 + +// SetAPITimeout records the resolved API timeout for subsequent CreateContext +// calls. A non-positive value resets to the default. +func SetAPITimeout(d time.Duration) { + if d <= 0 { + apiTimeoutNanos.Store(0) + return + } + apiTimeoutNanos.Store(int64(d)) +} + +// apiTimeout returns the configured API timeout, or the default if unset. +func apiTimeout() time.Duration { + if n := apiTimeoutNanos.Load(); n > 0 { + return time.Duration(n) + } + return domain.TimeoutAPI +} + +// CreateContext creates a context with the configured API timeout (see +// SetAPITimeout; defaults to domain.TimeoutAPI). // Returns the context and a cancel function that should be deferred. func CreateContext() (context.Context, context.CancelFunc) { - return context.WithTimeout(context.Background(), domain.TimeoutAPI) + return context.WithTimeout(context.Background(), apiTimeout()) } // CreateContextWithTimeout creates a context with a custom timeout. diff --git a/internal/cli/common/context_test.go b/internal/cli/common/context_test.go index ea35c40..ff0b145 100644 --- a/internal/cli/common/context_test.go +++ b/internal/cli/common/context_test.go @@ -4,9 +4,14 @@ import ( "context" "testing" "time" + + "github.com/nylas/cli/internal/domain" ) func TestCreateContext(t *testing.T) { + SetAPITimeout(0) // default + defer SetAPITimeout(0) + ctx, cancel := CreateContext() defer cancel() @@ -20,11 +25,37 @@ func TestCreateContext(t *testing.T) { t.Error("CreateContext() context has no deadline") } - // Check that deadline is approximately 90 seconds from now (TimeoutAPI) - expectedDeadline := time.Now().Add(90 * time.Second) + // Default deadline is TimeoutAPI from now. + expectedDeadline := time.Now().Add(domain.TimeoutAPI) diff := expectedDeadline.Sub(deadline) if diff < -1*time.Second || diff > 1*time.Second { - t.Errorf("CreateContext() deadline is %v, expected around 90s from now", deadline) + t.Errorf("CreateContext() deadline is %v, expected around %v from now", deadline, domain.TimeoutAPI) + } +} + +func TestCreateContext_HonorsConfiguredTimeout(t *testing.T) { + SetAPITimeout(45 * time.Second) + defer SetAPITimeout(0) + + ctx, cancel := CreateContext() + defer cancel() + + deadline, ok := ctx.Deadline() + if !ok { + t.Fatal("CreateContext() context has no deadline") + } + diff := time.Until(deadline) - 45*time.Second + if diff < -1*time.Second || diff > 1*time.Second { + t.Errorf("CreateContext() deadline ~%v from now, expected ~45s", time.Until(deadline)) + } + + // Resetting to default restores TimeoutAPI. + SetAPITimeout(0) + ctx2, cancel2 := CreateContext() + defer cancel2() + d2, _ := ctx2.Deadline() + if diff := time.Until(d2) - domain.TimeoutAPI; diff < -1*time.Second || diff > 1*time.Second { + t.Errorf("after reset, deadline ~%v, expected ~%v", time.Until(d2), domain.TimeoutAPI) } } diff --git a/internal/cli/slack/files.go b/internal/cli/slack/files.go index a785bd4..aa9ae26 100644 --- a/internal/cli/slack/files.go +++ b/internal/cli/slack/files.go @@ -13,6 +13,7 @@ import ( "github.com/nylas/cli/internal/cli/common" "github.com/nylas/cli/internal/domain" + "github.com/nylas/cli/internal/httputil" ) // newFilesCmd creates the files command group. @@ -330,10 +331,9 @@ Examples: return common.NewInputError(fmt.Sprintf("output path is a directory: %s", outputPath)) } - // Download the file with a dedicated long timeout: the command - // context carries the default API timeout, which would cut off - // large downloads mid-stream. - dlCtx, dlCancel := common.CreateContextWithTimeout(domain.TimeoutDownload) + // Download under the standard 120s client timeout (the Nylas + // server-side ceiling), matching email attachment downloads. + dlCtx, dlCancel := common.CreateContextWithTimeout(httputil.DefaultClientTimeout) defer dlCancel() reader, err := client.DownloadFile(dlCtx, file.DownloadURL) if err != nil { diff --git a/internal/cli/update/update.go b/internal/cli/update/update.go index 22484c0..dec293f 100644 --- a/internal/cli/update/update.go +++ b/internal/cli/update/update.go @@ -9,7 +9,6 @@ import ( "github.com/spf13/cobra" "github.com/nylas/cli/internal/cli/common" - "github.com/nylas/cli/internal/domain" "github.com/nylas/cli/internal/version" ) @@ -171,23 +170,3 @@ func runUpdate(ctx context.Context, checkOnly, force, yes bool) error { return nil } - -// CheckForUpdateAsync checks for updates in the background and prints a message if available. -// This can be called during CLI startup for non-blocking update notifications. -func CheckForUpdateAsync(currentVersion string) { - go func() { - ctx, cancel := common.CreateContextWithTimeout(domain.TimeoutQuickCheck) - defer cancel() - - release, err := getLatestRelease(ctx) - if err != nil { - return // Silently ignore errors in async check - } - - latestVersion := parseVersion(release.TagName) - if isUpdateAvailable(currentVersion, latestVersion) { - fmt.Printf("\nA new version of Nylas CLI is available: %s (current: %s)\n", latestVersion, currentVersion) - fmt.Println("Run 'nylas update' to upgrade.") - } - }() -} diff --git a/internal/domain/config.go b/internal/domain/config.go index be47b0a..d18750d 100644 --- a/internal/domain/config.go +++ b/internal/domain/config.go @@ -1,6 +1,7 @@ package domain import ( + "os" "strings" "time" ) @@ -8,8 +9,11 @@ import ( // Timeout constants for consistent behavior across the application. // Use these instead of hardcoding timeout values. const ( - // TimeoutAPI is the default timeout for Nylas API calls (90s). - TimeoutAPI = 90 * time.Second + // TimeoutAPI is the default timeout for Nylas API calls (120s), matching the + // Nylas server-side request timeout and httputil.DefaultClientTimeout. + // Override per-install via `nylas config set api.timeout` or NYLAS_API_TIMEOUT + // (see Config.ResolveAPITimeout). + TimeoutAPI = 120 * time.Second // TimeoutHealthCheck is the timeout for health/connectivity checks (10s). TimeoutHealthCheck = 10 * time.Second @@ -22,10 +26,6 @@ const ( // all Slack messages or channels (10m). TimeoutBulkOperation = 10 * time.Minute - // TimeoutDownload is the timeout for streamed file/attachment downloads (15m). - // Large or slow downloads would be cut off mid-stream by TimeoutAPI. - TimeoutDownload = 15 * time.Minute - // TimeoutQuickCheck is the timeout for quick checks like version checking (5s). TimeoutQuickCheck = 5 * time.Second @@ -65,6 +65,7 @@ type Config struct { // APIConfig represents API-specific configuration. type APIConfig struct { BaseURL string `yaml:"base_url,omitempty"` // API base URL + Timeout string `yaml:"timeout,omitempty"` // API request timeout, e.g. "120s" (default TimeoutAPI) } const ( @@ -83,6 +84,33 @@ func (c *Config) ResolveBaseURL() string { return BaseURLUS } +// ResolveAPITimeout returns the effective per-request API timeout. Order of +// precedence: NYLAS_API_TIMEOUT env var, then config api.timeout, then the +// TimeoutAPI default. Unparseable or non-positive values are ignored so a bad +// override degrades to the default rather than disabling the timeout. +func (c *Config) ResolveAPITimeout() time.Duration { + if d, ok := parsePositiveDuration(os.Getenv("NYLAS_API_TIMEOUT")); ok { + return d + } + if c.API != nil { + if d, ok := parsePositiveDuration(c.API.Timeout); ok { + return d + } + } + return TimeoutAPI +} + +func parsePositiveDuration(s string) (time.Duration, bool) { + if s == "" { + return 0, false + } + d, err := time.ParseDuration(s) + if err != nil || d <= 0 { + return 0, false + } + return d, true +} + // DefaultConfig returns a config with sensible defaults. func DefaultConfig() *Config { return &Config{ diff --git a/internal/domain/config_extended_test.go b/internal/domain/config_extended_test.go index 53fb158..2a00a43 100644 --- a/internal/domain/config_extended_test.go +++ b/internal/domain/config_extended_test.go @@ -5,6 +5,7 @@ package domain import ( "encoding/json" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -68,6 +69,34 @@ func TestResolveBaseURL(t *testing.T) { } } +func TestResolveAPITimeout(t *testing.T) { + t.Run("defaults to TimeoutAPI when unset", func(t *testing.T) { + t.Setenv("NYLAS_API_TIMEOUT", "") + assert.Equal(t, TimeoutAPI, (&Config{}).ResolveAPITimeout()) + assert.Equal(t, TimeoutAPI, (&Config{API: &APIConfig{}}).ResolveAPITimeout()) + }) + + t.Run("config api.timeout overrides default", func(t *testing.T) { + t.Setenv("NYLAS_API_TIMEOUT", "") + cfg := &Config{API: &APIConfig{Timeout: "45s"}} + assert.Equal(t, 45*time.Second, cfg.ResolveAPITimeout()) + }) + + t.Run("env overrides config", func(t *testing.T) { + t.Setenv("NYLAS_API_TIMEOUT", "3m") + cfg := &Config{API: &APIConfig{Timeout: "45s"}} + assert.Equal(t, 3*time.Minute, cfg.ResolveAPITimeout()) + }) + + t.Run("invalid or non-positive values fall back to default", func(t *testing.T) { + t.Setenv("NYLAS_API_TIMEOUT", "") + for _, bad := range []string{"banana", "0s", "-5s"} { + cfg := &Config{API: &APIConfig{Timeout: bad}} + assert.Equal(t, TimeoutAPI, cfg.ResolveAPITimeout(), "value %q should fall back", bad) + } + }) +} + func TestDefaultWorkingHours(t *testing.T) { schedule := DefaultWorkingHours() diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go index 1686f07..64d1f28 100644 --- a/internal/httputil/httputil.go +++ b/internal/httputil/httputil.go @@ -16,8 +16,10 @@ import ( // This prevents memory exhaustion attacks via large payloads. const MaxRequestBodySize = 1 << 20 -// DefaultClientTimeout is the standard timeout for outbound HTTP clients. -const DefaultClientTimeout = 120 * time.Second +// DefaultClientTimeout is the standard timeout for outbound HTTP clients. It +// tracks domain.TimeoutAPI (120s) so the client-level cap and the per-request +// API timeout share one source of truth. +const DefaultClientTimeout = domain.TimeoutAPI // userAgentTransport sets the standard CLI User-Agent on every outbound // request that doesn't already specify one, so all clients built via diff --git a/internal/tui/compose.go b/internal/tui/compose.go index 3cbe6d4..7fc3fa3 100644 --- a/internal/tui/compose.go +++ b/internal/tui/compose.go @@ -270,7 +270,7 @@ func (c *ComposeView) prefillForReply() { originalBody = msg.Snippet } // Strip HTML if present - originalBody = stripHTMLForQuote(originalBody) + originalBody = stripHTMLForTUI(originalBody) // Add > prefix to each line lines := strings.Split(originalBody, "\n") diff --git a/internal/tui/compose_actions.go b/internal/tui/compose_actions.go index 62525cd..1f241d4 100644 --- a/internal/tui/compose_actions.go +++ b/internal/tui/compose_actions.go @@ -212,11 +212,6 @@ func parseRecipients(input string) []domain.EmailParticipant { return recipients } -// stripHTMLForQuote removes HTML tags for quoting in replies. -func stripHTMLForQuote(s string) string { - return stripHTMLForTUI(s) -} - // convertToHTML converts plain text to HTML for proper email rendering. func convertToHTML(text string) string { // Escape HTML special characters diff --git a/internal/tui/dialog.go b/internal/tui/dialog.go index d18f86e..4ec1b99 100644 --- a/internal/tui/dialog.go +++ b/internal/tui/dialog.go @@ -7,15 +7,6 @@ import ( "github.com/rivo/tview" ) -// DialogType defines the type of dialog. -type DialogType int - -const ( - DialogConfirm DialogType = iota - DialogInfo - DialogError -) - // Dialog displays a modal dialog for confirmations or messages. type Dialog struct { *tview.Flex