internal/mcp/register_reload_tool.go (1)

18-19: Prefer slog.InfoContext for consistency

Other tool handlers log action execution with slog.InfoContext, while the registration log uses plain slog.Info. For consistency – and to keep correlation IDs – consider:

- slog.Info("Registering HAProxy reload tool...")
+ slog.InfoContext(context.TODO(), "Registering HAProxy reload tool")

Or simply drop it, as the enclosing caller already announces “Registering HAProxy MCP tools…”.

internal/haproxy/runtime/runtime.go (1)

73-80: parseCSVStats header is ignored – risk of silent format drift

parseCSVStats returns both header and rows.
By discarding the header entirely (_), the function silently accepts any future HAProxy column changes; downstream code will still index by map keys but may miss new columns or mis-interpret renamed ones.

Consider validating the header (e.g. ensure mandatory columns like pxname, svname, status are present) before accepting the data. This provides an early, explicit failure mode when HAProxy versions change.

internal/mcp/tools.go (1)

11-18: Duplicate boilerplate – register functions could be looped for cleaner scaling

As the number of tool categories grows, adding manual calls bloats RegisterTools. A small slice of registrar functions centralises the flow and keeps the file tiny:

- slog.Info("Registering HAProxy MCP tools...")
- registerStatTools(s, client)
- registerBackendTools(s, client)
- registerServerTools(s, client)
- registerHealthAgentTools(s, client)
- registerReloadTool(s, client)
- slog.Info("All HAProxy MCP tools registered successfully")
+ slog.Info("Registering HAProxy MCP tools…")
+ for _, f := range []func(*server.MCPServer, *haproxy.HAProxyClient){
+     registerStatTools,
+     registerBackendTools,
+     registerServerTools,
+     registerHealthAgentTools,
+     registerReloadTool,
+ } {
+     f(s, client)
+ }
+ slog.Info("All HAProxy MCP tools registered successfully")

This adds no complexity but reduces future merge conflicts when new categories appear.

internal/mcp/register_health_agent_tools.go (1)

17-83: Consider reducing duplication in tool handlers

The four tools (enable_health, disable_health, enable_agent, disable_agent) follow almost identical patterns. Consider refactoring to reduce duplication using a helper function that takes the tool name, description, and client method as parameters.

+ // healthAgentToolConfig holds configuration for a health/agent check tool
+ type healthAgentToolConfig struct {
+     name        string
+     description string
+     action      string
+     clientFunc  func(backend, server string) error
+     successMsg  string
+ }
+ 
+ // registerHealthAgentTool is a helper to register a health/agent check tool
+ func registerHealthAgentTool(s *server.MCPServer, config healthAgentToolConfig) {
+     tool := mcp.NewTool(config.name,
+         mcp.WithDescription(config.description),
+         mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
+         mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to "+config.action+" for")),
+     )
+     s.AddTool(tool, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+         backend := getString(req, "backend")
+         serverName := getString(req, "server")
+         slog.InfoContext(ctx, "Executing "+config.name, "backend", backend, "server", serverName)
+         return callExec(ctx, config.action, func() (string, error) {
+             if err := config.clientFunc(backend, serverName); err != nil {
+                 return "", err
+             }
+             return fmt.Sprintf(config.successMsg, backend, serverName), nil
+         })
+     })
+ }
+ 
  func registerHealthAgentTools(s *server.MCPServer, client *haproxy.HAProxyClient) {
      slog.Info("Registering HAProxy health & agent check tools...")
      
+     // Define tool configurations
+     tools := []healthAgentToolConfig{
+         {
+             name:        "enable_health",
+             description: "Enables health checks for a server in a backend",
+             action:      "enable health checks",
+             clientFunc:  client.EnableHealth,
+             successMsg:  "Health checks for server %s/%s enabled successfully",
+         },
+         {
+             name:        "disable_health",
+             description: "Disables health checks for a server in a backend",
+             action:      "disable health checks", 
+             clientFunc:  client.DisableHealth,
+             successMsg:  "Health checks for server %s/%s disabled successfully",
+         },
+         {
+             name:        "enable_agent",
+             description: "Enables agent checks for a server in a backend",
+             action:      "enable agent checks",
+             clientFunc:  client.EnableAgent,
+             successMsg:  "Agent checks for server %s/%s enabled successfully",
+         },
+         {
+             name:        "disable_agent",
+             description: "Disables agent checks for a server in a backend",
+             action:      "disable agent checks",
+             clientFunc:  client.DisableAgent,
+             successMsg:  "Agent checks for server %s/%s disabled successfully",
+         },
+     }
+     
+     // Register all tools based on configurations
+     for _, config := range tools {
+         registerHealthAgentTool(s, config)
+     }
-
-     enableHealth := mcp.NewTool("enable_health",
-         mcp.WithDescription("Enables health checks for a server in a backend"),
-         mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
-         mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to enable health checks for")),
-     )
-     s.AddTool(enableHealth, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-         backend := getString(req, "backend")
-         serverName := getString(req, "server")
-         slog.InfoContext(ctx, "Executing enable_health", "backend", backend, "server", serverName)
-         return callExec(ctx, "enable health checks", func() (string, error) {
-             if err := client.EnableHealth(backend, serverName); err != nil {
-                 return "", err
-             }
-             return fmt.Sprintf("Health checks for server %s/%s enabled successfully", backend, serverName), nil
-         })
-     })
-
-     disableHealth := mcp.NewTool("disable_health",
-         mcp.WithDescription("Disables health checks for a server in a backend"),
-         mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
-         mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to disable health checks for")),
-     )
-     s.AddTool(disableHealth, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-         backend := getString(req, "backend")
-         serverName := getString(req, "server")
-         slog.InfoContext(ctx, "Executing disable_health", "backend", backend, "server", serverName)
-         return callExec(ctx, "disable health checks", func() (string, error) {
-             if err := client.DisableHealth(backend, serverName); err != nil {
-                 return "", err
-             }
-             return fmt.Sprintf("Health checks for server %s/%s disabled successfully", backend, serverName), nil
-         })
-     })
-
-     enableAgent := mcp.NewTool("enable_agent",
-         mcp.WithDescription("Enables agent checks for a server in a backend"),
-         mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
-         mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to enable agent checks for")),
-     )
-     s.AddTool(enableAgent, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-         backend := getString(req, "backend")
-         serverName := getString(req, "server")
-         slog.InfoContext(ctx, "Executing enable_agent", "backend", backend, "server", serverName)
-         return callExec(ctx, "enable agent checks", func() (string, error) {
-             if err := client.EnableAgent(backend, serverName); err != nil {
-                 return "", err
-             }
-             return fmt.Sprintf("Agent checks for server %s/%s enabled successfully", backend, serverName), nil
-         })
-     })
-
-     disableAgent := mcp.NewTool("disable_agent",
-         mcp.WithDescription("Disables agent checks for a server in a backend"),
-         mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
-         mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to disable agent checks for")),
-     )
-     s.AddTool(disableAgent, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-         backend := getString(req, "backend")
-         serverName := getString(req, "server")
-         slog.InfoContext(ctx, "Executing disable_agent", "backend", backend, "server", serverName)
-         return callExec(ctx, "disable agent checks", func() (string, error) {
-             if err := client.DisableAgent(backend, serverName); err != nil {
-                 return "", err
-             }
-             return fmt.Sprintf("Agent checks for server %s/%s disabled successfully", backend, serverName), nil
-         })
-     })

internal/mcp/register_backend_tools.go (1)

40-50: Add validation for the optional backend parameter

The show_servers_state tool has an optional backend parameter, but there's no validation for its correctness if provided.

 showServersState := mcp.NewTool("show_servers_state",
     mcp.WithDescription("Shows the state of servers including sessions and weight"),
     mcp.WithString("backend", mcp.Description("Optional backend name to filter servers")),
 )
 s.AddTool(showServersState, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
     backend := getString(req, "backend")
+    // Log differently based on whether we're filtering by backend or showing all
+    if backend != "" {
+        slog.InfoContext(ctx, "Executing show_servers_state with backend filter", "backend", backend)
+    } else {
+        slog.InfoContext(ctx, "Executing show_servers_state for all backends")
+    }
-    slog.InfoContext(ctx, "Executing show_servers_state", "backend", backend)
     return callJSON(ctx, "show servers state", "servers_state", func() (interface{}, error) {
         return client.ShowServersState(backend)
     })
 })

internal/mcp/register_server_tools.go (2)

54-67: Validate optional parameters when non-zero

The add_server tool has optional port and weight parameters, but there's no validation to ensure they're valid when provided.

 s.AddTool(addServer, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
     backend := getString(req, "backend")
     name := getString(req, "name")
     addr := getString(req, "addr")
     port := getInt(req, "port")
     weight := getInt(req, "weight")
+    
+    // Log port and weight only when they're provided
+    logParams := []any{"backend", backend, "name", name, "addr", addr}
+    if port > 0 {
+        logParams = append(logParams, "port", port)
+    }
+    if weight > 0 {
+        logParams = append(logParams, "weight", weight)
+    }
+    slog.InfoContext(ctx, "Executing add_server", logParams...)
-    slog.InfoContext(ctx, "Executing add_server", "backend", backend, "name", name, "addr", addr, "port", port, "weight", weight)
     
     return callExec(ctx, "add server", func() (string, error) {
         if err := client.AddServer(backend, name, addr, port, weight); err != nil {
             return "", err
         }
         return fmt.Sprintf("Server %s added successfully to backend %s", name, backend), nil
     })
 })

---

86-121: Code duplication between enable_server and disable_server

Similar to the health and agent check tools, the enable_server and disable_server tools have very similar code. Consider refactoring to reduce duplication.

+// serverToggleConfig holds configuration for a server enable/disable tool
+type serverToggleConfig struct {
+    name        string
+    description string
+    action      string
+    clientFunc  func(backend, server string) error
+}
+
+// registerServerToggleTool is a helper to register a server enable/disable tool
+func registerServerToggleTool(s *server.MCPServer, config serverToggleConfig) {
+    tool := mcp.NewTool(config.name,
+        mcp.WithDescription(config.description),
+        mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
+        mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to "+config.action)),
+    )
+    s.AddTool(tool, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+        backend := getString(req, "backend")
+        serverName := getString(req, "server")
+        slog.InfoContext(ctx, "Executing "+config.name, "backend", backend, "server", serverName)
+        return callExec(ctx, config.action+" server", func() (string, error) {
+            if err := config.clientFunc(backend, serverName); err != nil {
+                return "", err
+            }
+            // The past participle of enable is "enabled", disable is "disabled"
+            actionPastParticiple := config.action + "d"
+            return fmt.Sprintf("Server %s/%s %s successfully", backend, serverName, actionPastParticiple), nil
+        })
+    })
+}

 // In registerServerTools:
-// enable_server tool
-enableServer := mcp.NewTool("enable_server",
-    mcp.WithDescription("Enables a server in a backend"),
-    mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
-    mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to enable")),
-)
-s.AddTool(enableServer, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-    backend := getString(req, "backend")
-    serverName := getString(req, "server")
-    slog.InfoContext(ctx, "Executing enable_server", "backend", backend, "server", serverName)
-    return callExec(ctx, "enable server", func() (string, error) {
-        if err := client.EnableServer(backend, serverName); err != nil {
-            return "", err
-        }
-        return fmt.Sprintf("Server %s/%s enabled successfully", backend, serverName), nil
-    })
-})
-
-// disable_server tool
-disableServer := mcp.NewTool("disable_server",
-    mcp.WithDescription("Disables a server in a backend"),
-    mcp.WithString("backend", mcp.Required(), mcp.Description("Name of the backend containing the server")),
-    mcp.WithString("server", mcp.Required(), mcp.Description("Name of the server to disable")),
-)
-s.AddTool(disableServer, func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-    backend := getString(req, "backend")
-    serverName := getString(req, "server")
-    slog.InfoContext(ctx, "Executing disable_server", "backend", backend, "server", serverName)
-    return callExec(ctx, "disable server", func() (string, error) {
-        if err := client.DisableServer(backend, serverName); err != nil {
-            return "", err
-        }
-        return fmt.Sprintf("Server %s/%s disabled successfully", backend, serverName), nil
-    })
-})
+// Register server toggle tools
+registerServerToggleTool(s, serverToggleConfig{
+    name:        "enable_server",
+    description: "Enables a server in a backend",
+    action:      "enable",
+    clientFunc:  client.EnableServer,
+})
+
+registerServerToggleTool(s, serverToggleConfig{
+    name:        "disable_server",
+    description: "Disables a server in a backend",
+    action:      "disable",
+    clientFunc:  client.DisableServer,
+})

internal/haproxy/client.go (2)

196-210: Consider refactoring SetWeight to use the ensureRuntime helper.

While most methods now use the centralized ensureRuntime helper for runtime client validation, this method still uses direct checking. For consistency, consider refactoring.

func (c *HAProxyClient) SetWeight(backend, server string, weight int) (string, error) {
-	if c.RuntimeClient == nil {
-		return "", fmt.Errorf("runtime client is not initialized")
-	}
+	if err := c.ensureRuntime(); err != nil {
+		return "", err
+	}

	// Directly execute the command since it might be different across versions
	cmd := fmt.Sprintf("set weight %s/%s %d", backend, server, weight)
	_, err := c.RuntimeClient.ExecuteRuntimeCommand(cmd)
	if err != nil {
		return "", err
	}

	return fmt.Sprintf("Weight for %s/%s set to %d", backend, server, weight), nil
}

---

339-351: Consider refactoring remaining methods to use the ensureRuntime helper.

Several methods (DumpStatsFile, DebugCounters, ClearCountersAll, AddServer, DelServer, ReloadHAProxy) still use direct checking for runtime client initialization. Consider refactoring them to use the centralized ensureRuntime helper for consistency with the rest of the codebase.

For example, for the DumpStatsFile method:

func (c *HAProxyClient) DumpStatsFile(filepath string) (string, error) {
-	if c.RuntimeClient == nil {
-		return "", fmt.Errorf("runtime client is not initialized")
-	}
+	if err := c.ensureRuntime(); err != nil {
+		return "", err
+	}

	cmd := fmt.Sprintf("show stat > %s", filepath)
	_, err := c.RuntimeClient.ExecuteRuntimeCommand(cmd)
	if err != nil {
		return "", err
	}

	return filepath, nil
}

Apply similar refactoring to the other mentioned methods.

Also applies to: 354-385, 388-403, 406-414, 417-427

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro