refactor(oauth): address code review suggestions for token refresh

- Extract containsIgnoreCase to new internal/stringutil package
- Replace custom toLower implementations with strings.ToLower
- Add MaxExpiredTokenAge constant (24h) with documentation
- Add TestRefreshStateSync to ensure health and oauth RefreshState stay in sync

These changes improve code maintainability by:
- Eliminating duplicate containsIgnoreCase implementations
- Using stdlib strings.ToLower instead of custom ASCII-only version
- Making the 24-hour expiry threshold explicit and documented
- Adding a guard test to catch RefreshState constant drift

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: technicalpickles

internal/health/calculator.go

@@ -3,9 +3,11 @@ package health
 
 import (
 	"fmt"
+	"strings"
 	"time"
 
 	"mcpproxy-go/internal/contracts"
+	"mcpproxy-go/internal/stringutil"
 )
 
 // RefreshState represents the current state of token refresh for health reporting.
@@ -125,7 +127,7 @@ func CalculateHealth(input HealthCalculatorInput, cfg *HealthCalculatorConfig) *
 	// 4. Connection state checks
 	// Normalize state to lowercase for consistent matching
 	// (ConnectionState.String() returns "Error", "Disconnected", etc.)
-	state := toLower(input.State)
+	state := strings.ToLower(input.State)
 	switch state {
 	case "error":
 		// For OAuth-required servers with OAuth-related errors, suggest login instead of restart
@@ -320,7 +322,7 @@ func formatErrorSummary(lastError string) string {
 
 	// Check for known patterns (in order)
 	for _, mapping := range errorMappings {
-		if containsIgnoreCase(lastError, mapping.pattern) {
+		if stringutil.ContainsIgnoreCase(lastError, mapping.pattern) {
 			return mapping.friendly
 		}
 	}
@@ -375,36 +377,6 @@ func formatRefreshRetryDetail(retryCount int, nextAttempt *time.Time, lastError
 	return detail
 }
 
-// containsIgnoreCase checks if s contains substr, ignoring case.
-func containsIgnoreCase(s, substr string) bool {
-	return len(s) >= len(substr) &&
-		(s == substr ||
-		 containsLower(toLower(s), toLower(substr)))
-}
-
-// toLower is a simple ASCII lowercase conversion.
-func toLower(s string) string {
-	b := make([]byte, len(s))
-	for i := 0; i < len(s); i++ {
-		c := s[i]
-		if c >= 'A' && c <= 'Z' {
-			c += 'a' - 'A'
-		}
-		b[i] = c
-	}
-	return string(b)
-}
-
-// containsLower checks if s contains substr (both should be lowercase).
-func containsLower(s, substr string) bool {
-	for i := 0; i <= len(s)-len(substr); i++ {
-		if s[i:i+len(substr)] == substr {
-			return true
-		}
-	}
-	return false
-}
-
 // isOAuthRelatedError checks if the error message indicates an OAuth issue.
 func isOAuthRelatedError(err string) bool {
 	if err == "" {
@@ -422,7 +394,7 @@ func isOAuthRelatedError(err string) bool {
 		"access_denied",
 	}
 	for _, pattern := range oauthPatterns {
-		if containsIgnoreCase(err, pattern) {
+		if stringutil.ContainsIgnoreCase(err, pattern) {
 			return true
 		}
 	}
@@ -475,7 +447,7 @@ func ExtractOAuthConfigError(lastError string) string {
 	}
 
 	for _, pattern := range configPatterns {
-		if containsIgnoreCase(lastError, pattern) {
+		if stringutil.ContainsIgnoreCase(lastError, pattern) {
 			return lastError
 		}
 	}

internal/health/calculator_test.go

@@ -5,6 +5,8 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+
+	"mcpproxy-go/internal/oauth"
 )
 
 func TestCalculateHealth_DisabledServer(t *testing.T) {
@@ -762,3 +764,19 @@ func TestFormatRefreshRetryDetail(t *testing.T) {
 		assert.LessOrEqual(t, len(result), 200) // Reasonable max length
 	})
 }
+
+// TestRefreshStateSync ensures health.RefreshState values stay in sync with oauth.RefreshState.
+// The health package mirrors oauth.RefreshState for decoupling, but the values must match
+// for proper state mapping when wiring RefreshManager state into health calculation.
+func TestRefreshStateSync(t *testing.T) {
+	// Verify that the integer values match between health and oauth packages
+	// This test will fail if either package changes its constants without updating the other
+	assert.Equal(t, int(RefreshStateIdle), int(oauth.RefreshStateIdle),
+		"RefreshStateIdle values must match between health and oauth packages")
+	assert.Equal(t, int(RefreshStateScheduled), int(oauth.RefreshStateScheduled),
+		"RefreshStateScheduled values must match between health and oauth packages")
+	assert.Equal(t, int(RefreshStateRetrying), int(oauth.RefreshStateRetrying),
+		"RefreshStateRetrying values must match between health and oauth packages")
+	assert.Equal(t, int(RefreshStateFailed), int(oauth.RefreshStateFailed),
+		"RefreshStateFailed values must match between health and oauth packages")
+}

internal/oauth/refresh_manager.go

@@ -10,6 +10,7 @@ import (
 	"go.uber.org/zap"
 
 	"mcpproxy-go/internal/storage"
+	"mcpproxy-go/internal/stringutil"
 )
 
 // Default refresh configuration
@@ -31,6 +32,11 @@ const (
 
 	// MaxRetryBackoff is the maximum backoff duration (5 minutes per FR-009).
 	MaxRetryBackoff = 5 * time.Minute
+
+	// MaxExpiredTokenAge is how long after token expiration we continue retrying
+	// before giving up completely. After this duration, we assume the refresh token
+	// is no longer valid even if it wasn't explicitly rejected.
+	MaxExpiredTokenAge = 24 * time.Hour
 )
 
 // RefreshState represents the current state of token refresh for health reporting.
@@ -656,9 +662,9 @@ func (m *RefreshManager) handleRefreshFailure(serverName string, err error) {
 	if !expiresAt.IsZero() && now.After(expiresAt) {
 		// Token has already expired - check if we should give up
 		// We'll keep trying as long as there's a chance the refresh token is still valid
-		// Only give up if we've been trying for a very long time (e.g., > 24 hours past expiration)
+		// Only give up if we've been trying for too long (MaxExpiredTokenAge)
 		timeSinceExpiry := now.Sub(expiresAt)
-		if timeSinceExpiry > 24*time.Hour {
+		if timeSinceExpiry > MaxExpiredTokenAge {
 			m.logger.Error("OAuth token refresh failed - token expired too long ago",
 				zap.String("server", serverName),
 				zap.Duration("expired_for", timeSinceExpiry),
@@ -699,7 +705,7 @@ func classifyRefreshError(err error) string {
 		"refresh token invalid",
 	}
 	for _, pattern := range permanentErrors {
-		if containsIgnoreCase(errStr, pattern) {
+		if stringutil.ContainsIgnoreCase(errStr, pattern) {
 			return "failed_invalid_grant"
 		}
 	}
@@ -716,39 +722,14 @@ func classifyRefreshError(err error) string {
 		"context deadline exceeded",
 	}
 	for _, pattern := range networkErrors {
-		if containsIgnoreCase(errStr, pattern) {
+		if stringutil.ContainsIgnoreCase(errStr, pattern) {
 			return "failed_network"
 		}
 	}
 
 	return "failed_other"
 }
 
-// containsIgnoreCase checks if s contains substr, ignoring case.
-func containsIgnoreCase(s, substr string) bool {
-	sLower := toLower(s)
-	substrLower := toLower(substr)
-	for i := 0; i <= len(sLower)-len(substrLower); i++ {
-		if sLower[i:i+len(substrLower)] == substrLower {
-			return true
-		}
-	}
-	return false
-}
-
-// toLower converts a string to lowercase (ASCII only).
-func toLower(s string) string {
-	b := make([]byte, len(s))
-	for i := 0; i < len(s); i++ {
-		c := s[i]
-		if c >= 'A' && c <= 'Z' {
-			c += 'a' - 'A'
-		}
-		b[i] = c
-	}
-	return string(b)
-}
-
 // calculateBackoff calculates the exponential backoff duration for a given retry count.
 // The formula is: base * 2^retryCount, capped at MaxRetryBackoff (5 minutes).
 // Sequence: 10s → 20s → 40s → 80s → 160s → 300s (cap).

internal/stringutil/strings.go

@@ -0,0 +1,9 @@
+// Package stringutil provides common string utility functions.
+package stringutil
+
+import "strings"
+
+// ContainsIgnoreCase checks if s contains substr, ignoring case.
+func ContainsIgnoreCase(s, substr string) bool {
+	return strings.Contains(strings.ToLower(s), strings.ToLower(substr))
+}

internal/stringutil/strings_test.go

@@ -0,0 +1,35 @@
+package stringutil
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestContainsIgnoreCase(t *testing.T) {
+	tests := []struct {
+		name     string
+		s        string
+		substr   string
+		expected bool
+	}{
+		{"exact match", "hello", "hello", true},
+		{"case insensitive match", "Hello World", "hello", true},
+		{"case insensitive match upper", "hello world", "WORLD", true},
+		{"mixed case", "HeLLo WoRLD", "ello wor", true},
+		{"no match", "hello", "goodbye", false},
+		{"empty substr", "hello", "", true},
+		{"empty string", "", "hello", false},
+		{"both empty", "", "", true},
+		{"substr longer than string", "hi", "hello", false},
+		{"special chars", "error: invalid_grant", "INVALID_GRANT", true},
+		{"network error", "connection timeout", "TIMEOUT", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := ContainsIgnoreCase(tt.s, tt.substr)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}