Refactor configuration defaults and improve error handling

- Introduced a constant for the default port in config.go to enhance maintainability.
- Updated DefaultConfig to use the new constant for setting the Listen address.
- Improved error handling in loader.go by changing file write permissions for security.
- Added a testServerConnection method in e2e_test.go to verify server readiness more effectively.
- Refactored function signatures in various files for clarity and consistency.

Author: Dumbris

internal/config/config.go

@@ -5,6 +5,10 @@ import (
 	"time"
 )
 
+const (
+	defaultPort = ":8080"
+)
+
 // Config represents the main configuration structure
 type Config struct {
 	Listen            string          `json:"listen" mapstructure:"listen"`
@@ -140,7 +144,7 @@ type ToolStatEntry struct {
 // DefaultConfig returns a default configuration
 func DefaultConfig() *Config {
 	return &Config{
-		Listen:            ":8080",
+		Listen:            defaultPort,
 		DataDir:           "", // Will be set to ~/.mcpproxy by loader
 		EnableTray:        true,
 		DebugSearch:       false,
@@ -176,7 +180,7 @@ func DefaultConfig() *Config {
 // Validate validates the configuration
 func (c *Config) Validate() error {
 	if c.Listen == "" {
-		c.Listen = ":8080"
+		c.Listen = defaultPort
 	}
 	if c.TopK <= 0 {
 		c.TopK = 5

internal/config/loader.go

@@ -153,7 +153,7 @@ func setupViper() {
 }
 
 // findAndLoadConfigFile tries to find config file in common locations
-func findAndLoadConfigFile(cfg *Config) (bool, string, error) {
+func findAndLoadConfigFile(cfg *Config) (found bool, path string, err error) {
 	// Common config file locations
 	locations := []string{
 		ConfigFileName,
@@ -247,7 +247,7 @@ func SaveConfig(cfg *Config, path string) error {
 	}
 
 	fmt.Printf("[DEBUG] SaveConfig - about to write file: %s\n", path)
-	if err := os.WriteFile(path, data, 0644); err != nil {
+	if err := os.WriteFile(path, data, 0600); err != nil {
 		fmt.Printf("[DEBUG] SaveConfig - WriteFile failed: %v\n", err)
 		return fmt.Errorf("failed to write config file: %w", err)
 	}
@@ -325,9 +325,7 @@ func CreateSampleConfig(path string) error {
 }
 
 // Helper function to get current time (useful for testing)
-var now = func() time.Time {
-	return time.Now()
-}
+var now = time.Now
 
 // createDefaultConfigFile creates a default configuration file with default settings
 func createDefaultConfigFile(path string, cfg *Config) error {

internal/server/e2e_test.go

@@ -136,14 +136,41 @@ func (env *TestEnvironment) waitForServerReady() {
 			env.t.Fatalf("Timeout waiting for server to be ready. Status: %+v", status)
 		case <-ticker.C:
 			if env.proxyServer.IsRunning() {
-				// Give it a bit more time to fully initialize
-				time.Sleep(2 * time.Second)
-				return
+				// Actually test if the HTTP server is accepting connections
+				if env.testServerConnection() {
+					// Give it a bit more time to fully initialize
+					time.Sleep(500 * time.Millisecond)
+					return
+				}
 			}
 		}
 	}
 }
 
+// testServerConnection tests if the server is actually accepting HTTP connections
+func (env *TestEnvironment) testServerConnection() bool {
+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+	defer cancel()
+
+	req, err := http.NewRequestWithContext(ctx, "GET", env.proxyAddr, http.NoBody)
+	if err != nil {
+		return false
+	}
+
+	client := &http.Client{
+		Timeout: 1 * time.Second,
+	}
+
+	resp, err := client.Do(req)
+	if err != nil {
+		return false
+	}
+	resp.Body.Close()
+
+	// Any response (even an error response) means the server is accepting connections
+	return true
+}
+
 // CreateMockUpstreamServer creates and starts a mock upstream MCP server
 func (env *TestEnvironment) CreateMockUpstreamServer(name string, tools []mcp.Tool) *MockUpstreamServer {
 	// Create MCP server

internal/server/mcp.go

@@ -722,7 +722,7 @@ func (p *MCPProxyServer) handleInspectQuarantinedTools(ctx context.Context, requ
 				toolAnalysis := map[string]interface{}{
 					"name":              tool.Name,
 					"full_name":         fmt.Sprintf("%s:%s", serverName, tool.Name),
-					"description":       fmt.Sprintf("\"%s\"", tool.Description), // Quote the description for LLM analysis
+					"description":       fmt.Sprintf("%q", tool.Description), // Quote the description for LLM analysis
 					"input_schema":      inputSchema,
 					"server_name":       serverName,
 					"quarantine_status": "QUARANTINED",
@@ -1213,7 +1213,7 @@ func (p *MCPProxyServer) analyzeQuery(query string) map[string]interface{} {
 }
 
 // explainToolRanking explains why a specific tool was ranked as it was
-func (p *MCPProxyServer) explainToolRanking(query string, targetTool string, results []*config.SearchResult) map[string]interface{} {
+func (p *MCPProxyServer) explainToolRanking(query, targetTool string, results []*config.SearchResult) map[string]interface{} {
 	explanation := map[string]interface{}{
 		"target_tool":      targetTool,
 		"query":            query,
@@ -1223,18 +1223,19 @@ func (p *MCPProxyServer) explainToolRanking(query string, targetTool string, res
 
 	// Find the tool in results
 	for i, result := range results {
-		if result.Tool.Name == targetTool {
-			explanation["found_in_results"] = true
-			explanation["rank"] = i + 1
-			explanation["score"] = result.Score
-			explanation["tool_details"] = map[string]interface{}{
-				"name":        result.Tool.Name,
-				"server":      result.Tool.ServerName,
-				"description": result.Tool.Description,
-				"has_params":  len(result.Tool.ParamsJSON) > 0,
-			}
-			break
+		if result.Tool.Name != targetTool {
+			continue
+		}
+		explanation["found_in_results"] = true
+		explanation["rank"] = i + 1
+		explanation["score"] = result.Score
+		explanation["tool_details"] = map[string]interface{}{
+			"name":        result.Tool.Name,
+			"server":      result.Tool.ServerName,
+			"description": result.Tool.Description,
+			"has_params":  result.Tool.ParamsJSON != "",
 		}
+		break
 	}
 
 	// Analyze why tool might not rank well
@@ -1256,8 +1257,9 @@ func (p *MCPProxyServer) explainToolRanking(query string, targetTool string, res
 	if strings.Contains(targetTool, ":") {
 		parts := strings.SplitN(targetTool, ":", 2)
 		if len(parts) == 2 {
-			suggestions = append(suggestions, fmt.Sprintf("Try searching for server name: '%s'", parts[0]))
-			suggestions = append(suggestions, fmt.Sprintf("Try searching for tool name: '%s'", parts[1]))
+			suggestions = append(suggestions,
+				fmt.Sprintf("Try searching for server name: '%s'", parts[0]),
+				fmt.Sprintf("Try searching for tool name: '%s'", parts[1]))
 			if strings.Contains(parts[1], "_") {
 				words := strings.Split(parts[1], "_")
 				suggestions = append(suggestions, fmt.Sprintf("Try searching for individual words: '%s'", strings.Join(words, " ")))

internal/server/server.go

@@ -292,6 +292,8 @@ func (s *Server) backgroundToolIndexing(ctx context.Context) {
 
 // loadConfiguredServers synchronizes the storage and upstream manager from the current config.
 // This is the source of truth when configuration is loaded from disk.
+//
+//nolint:unparam // function designed to be best-effort, always returns nil by design
 func (s *Server) loadConfiguredServers() error {
 	s.logger.Info("Synchronizing servers from configuration (config as source of truth)")
 
@@ -764,7 +766,9 @@ func (s *Server) StopServer() error {
 	defer s.mu.Unlock()
 
 	if !s.running {
-		return fmt.Errorf("server is not running")
+		// Return nil instead of error to prevent race condition logs
+		s.logger.Debug("Server stop requested but server is not running")
+		return nil
 	}
 
 	// Notify about server stopping

internal/storage/bbolt.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"go.etcd.io/bbolt"
+	"go.etcd.io/bbolt/errors"
 	"go.uber.org/zap"
 )
 
@@ -30,7 +31,7 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) {
 		logger.Warnf("Failed to open database on first attempt: %v", err)
 
 		// Check if it's a timeout or lock issue
-		if err == bbolt.ErrTimeout {
+		if err == errors.ErrTimeout {
 			logger.Info("Database timeout detected, attempting recovery...")
 
 			// Try to backup and recreate if file exists

internal/tray/managers.go

@@ -12,6 +12,13 @@ import (
 	"go.uber.org/zap"
 )
 
+const (
+	actionEnable  = "enable"
+	actionDisable = "disable"
+	textEnable    = "Enable"
+	textDisable   = "Disable"
+)
+
 // ServerStateManager manages server state synchronization between storage, config, and menu
 type ServerStateManager struct {
 	server ServerInterface
@@ -150,9 +157,9 @@ func (m *ServerStateManager) UnquarantineServer(serverName string) error {
 
 // EnableServer enables/disables a server and ensures all state is synchronized
 func (m *ServerStateManager) EnableServer(serverName string, enabled bool) error {
-	action := "disable"
+	action := actionDisable
 	if enabled {
-		action = "enable"
+		action = actionEnable
 	}
 
 	m.logger.Info("EnableServer called",
@@ -274,16 +281,17 @@ func (m *MenuManager) UpdateUpstreamServersMenu(servers []map[string]interface{}
 
 	// --- Create Menu Items for New Servers ---
 	for serverName, serverData := range currentServerMap {
-		if _, exists := m.serverMenuItems[serverName]; !exists {
-			// This is a new server, create its menu item
-			m.logger.Info("Creating menu item for new server", zap.String("server", serverName))
-			status, tooltip := m.getServerStatusDisplay(serverData)
-			serverMenuItem := m.upstreamServersMenu.AddSubMenuItem(status, tooltip)
-			m.serverMenuItems[serverName] = serverMenuItem
-
-			// Create its action submenus
-			m.createServerActionSubmenus(serverMenuItem, serverData)
+		if _, exists := m.serverMenuItems[serverName]; exists {
+			continue
 		}
+		// This is a new server, create its menu item
+		m.logger.Info("Creating menu item for new server", zap.String("server", serverName))
+		status, tooltip := m.getServerStatusDisplay(serverData)
+		serverMenuItem := m.upstreamServersMenu.AddSubMenuItem(status, tooltip)
+		m.serverMenuItems[serverName] = serverMenuItem
+
+		// Create its action submenus
+		m.createServerActionSubmenus(serverMenuItem, serverData)
 	}
 }
 
@@ -391,7 +399,7 @@ func (m *MenuManager) ForceRefresh() {
 }
 
 // getServerStatusDisplay returns display text and tooltip for a server
-func (m *MenuManager) getServerStatusDisplay(server map[string]interface{}) (string, string) {
+func (m *MenuManager) getServerStatusDisplay(server map[string]interface{}) (displayText, tooltip string) {
 	serverName, _ := server["name"].(string)
 	enabled, _ := server["enabled"].(bool)
 	connected, _ := server["connected"].(bool)
@@ -415,10 +423,10 @@ func (m *MenuManager) getServerStatusDisplay(server map[string]interface{}) (str
 		statusText = "disconnected"
 	}
 
-	displayText := fmt.Sprintf("%s %s", statusIcon, serverName)
-	tooltip := fmt.Sprintf("%s - %s", serverName, statusText)
+	displayText = fmt.Sprintf("%s %s", statusIcon, serverName)
+	tooltip = fmt.Sprintf("%s - %s", serverName, statusText)
 
-	return displayText, tooltip
+	return
 }
 
 // createServerActionSubmenus creates action submenus for a server (enable/disable, quarantine)
@@ -434,9 +442,9 @@ func (m *MenuManager) createServerActionSubmenus(serverMenuItem *systray.MenuIte
 	// Enable/Disable action
 	var enableText string
 	if enabled {
-		enableText = "Disable"
+		enableText = textDisable
 	} else {
-		enableText = "Enable"
+		enableText = textEnable
 	}
 	enableItem := serverMenuItem.AddSubMenuItem(enableText, fmt.Sprintf("%s server %s", enableText, serverName))
 	m.serverActionItems[serverName] = enableItem
@@ -477,9 +485,9 @@ func (m *MenuManager) updateServerActionMenus(serverName string, server map[stri
 	if actionItem, exists := m.serverActionItems[serverName]; exists {
 		var enableText string
 		if enabled {
-			enableText = "Disable"
+			enableText = textDisable
 		} else {
-			enableText = "Enable"
+			enableText = textEnable
 		}
 		actionItem.SetTitle(enableText)
 		actionItem.SetTooltip(fmt.Sprintf("%s server %s", enableText, serverName))

internal/tray/tray.go

@@ -24,7 +24,8 @@ import (
 )
 
 const (
-	repo = "smart-mcp-proxy/mcpproxy-go" // Actual repository
+	repo     = "smart-mcp-proxy/mcpproxy-go" // Actual repository
+	osDarwin = "darwin"
 )
 
 //go:embed icon-mono-44.png
@@ -279,7 +280,7 @@ func (a *App) cleanup() {
 func (a *App) onReady() {
 	systray.SetIcon(iconData)
 	// On macOS, also set as template icon for better system integration
-	if runtime.GOOS == "darwin" {
+	if runtime.GOOS == osDarwin {
 		systray.SetTemplateIcon(iconData, iconData)
 	}
 	a.updateTooltip()
@@ -448,7 +449,7 @@ func (a *App) updateTooltipFromStatusData(status map[string]interface{}) {
 }
 
 // updateServersMenuFromStatusData is a legacy method, functionality is now in MenuManager
-func (a *App) updateServersMenuFromStatusData(status map[string]interface{}) {
+func (a *App) updateServersMenuFromStatusData(_ map[string]interface{}) {
 	// This function is kept for reference during transition but the primary
 	// logic is now handled by the MenuManager and SynchronizationManager.
 	// We trigger a sync instead of manually updating here.
@@ -534,7 +535,7 @@ func (a *App) checkForUpdates() {
 // getLatestRelease fetches the latest release information from GitHub
 func (a *App) getLatestRelease() (*GitHubRelease, error) {
 	url := fmt.Sprintf("https://api.github.com/repos/%s/releases/latest", repo)
-	resp, err := http.Get(url)
+	resp, err := http.Get(url) // #nosec G107 -- URL is constructed from known repo constant
 	if err != nil {
 		return nil, err
 	}
@@ -560,7 +561,7 @@ func (a *App) findAssetURL(release *GitHubRelease) (string, error) {
 
 // downloadAndApplyUpdate downloads and applies the update
 func (a *App) downloadAndApplyUpdate(url string) error {
-	resp, err := http.Get(url)
+	resp, err := http.Get(url) // #nosec G107 -- URL is from GitHub releases API which is trusted
 	if err != nil {
 		return err
 	}
@@ -599,15 +600,17 @@ func (a *App) applyZipUpdate(body io.Reader) error {
 	}
 
 	for _, f := range r.File {
-		if !f.FileInfo().IsDir() {
-			rc, err := f.Open()
-			if err != nil {
-				return err
-			}
-			defer rc.Close()
-
-			return update.Apply(rc, update.Options{TargetPath: executablePath})
+		if f.FileInfo().IsDir() {
+			continue
+		}
+		rc, err := f.Open()
+		if err != nil {
+			return err
 		}
+
+		err = update.Apply(rc, update.Options{TargetPath: executablePath})
+		rc.Close()
+		return err
 	}
 
 	return fmt.Errorf("no file found in zip archive to apply")
@@ -655,7 +658,7 @@ func (a *App) refreshMenusImmediate() {
 }
 
 // handleServerAction is the centralized handler for all server-related menu actions.
-func (a *App) handleServerAction(serverName string, action string) {
+func (a *App) handleServerAction(serverName, action string) {
 	var err error
 	a.logger.Info("Handling server action", zap.String("server", serverName), zap.String("action", action))
 

internal/tray/tray_test.go

@@ -45,7 +45,7 @@ func (m *MockServerInterface) GetUpstreamStats() map[string]interface{} {
 	return m.upstreamStats
 }
 
-func (m *MockServerInterface) StartServer(ctx context.Context) error {
+func (m *MockServerInterface) StartServer(_ context.Context) error {
 	m.running = true
 	return nil
 }