refactor: add helper functions for MCP response generation

Amolith created 2 months ago

- Add createErrorResult() and createSuccessResult() helper functions to reduce code duplication
- Add formatGoalText() helper for consistent goal text formatting
- Refactor all MCP handler functions to use helpers instead of inline response creation
- Reduces codebase by ~150 lines (23% reduction in server.go) while improving maintainability
- Provides single point of change for response format modifications
- Ensures consistent error message formatting across all handlers

Change summary

internal/mcp/helpers.go |  44 ++++++
internal/mcp/server.go  | 304 ++++--------------------------------------
2 files changed, 76 insertions(+), 272 deletions(-)

Detailed changes

internal/mcp/helpers.go

@@ -0,0 +1,44 @@
+// SPDX-FileCopyrightText: Amolith <[e-mail]>
+//
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package mcp
+
+import (
+	"fmt"
+
+	"github.com/mark3labs/mcp-go/mcp"
+)
+
+// createErrorResult creates a standardized error response for MCP tool calls
+func createErrorResult(message string) *mcp.CallToolResult {
+	return &mcp.CallToolResult{
+		Content: []mcp.Content{
+			mcp.TextContent{
+				Type: "text",
+				Text: message,
+			},
+		},
+		IsError: true,
+	}
+}
+
+// createSuccessResult creates a standardized success response for MCP tool calls
+func createSuccessResult(message string) *mcp.CallToolResult {
+	return &mcp.CallToolResult{
+		Content: []mcp.Content{
+			mcp.TextContent{
+				Type: "text",
+				Text: message,
+			},
+		},
+	}
+}
+
+// formatGoalText formats goal title and description into a consistent display format
+func formatGoalText(title, description string) string {
+	if description == "" {
+		return title
+	}
+	return fmt.Sprintf("%s: %s", title, description)
+}

internal/mcp/server.go

@@ -169,57 +169,23 @@ func (s *Server) handleSetGoal(ctx context.Context, request mcp.CallToolRequest)
 	arguments := request.GetArguments()
 	title, ok := arguments["title"].(string)
 	if !ok || title == "" {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: title parameter is required and must be a string",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: title parameter is required and must be a string"), nil
 	}
 
 	description, ok := arguments["description"].(string)
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: description parameter is required and must be a string",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: description parameter is required and must be a string"), nil
 	}
 
 	// Set goal
 	if err := s.planner.SetGoal(title, description); err != nil {
 		s.logger.Error("Failed to set goal", "error", err)
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: fmt.Sprintf("Error setting goal: %v", err),
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult(fmt.Sprintf("Error setting goal: %v", err)), nil
 	}
 
-	goalText := title
-	if description != "" {
-		goalText = fmt.Sprintf("%s: %s", title, description)
-	}
+	goalText := formatGoalText(title, description)
 	response := fmt.Sprintf("Goal \"%s\" saved! You probably want to add one or more tasks now.", goalText)
-	return &mcp.CallToolResult{
-		Content: []mcp.Content{
-			mcp.TextContent{
-				Type: "text",
-				Text: response,
-			},
-		},
-	}, nil
+	return createSuccessResult(response), nil
 }
 
 // handleChangeGoal handles the project_management__change_goal tool call
@@ -230,70 +196,28 @@ func (s *Server) handleChangeGoal(ctx context.Context, request mcp.CallToolReque
 	arguments := request.GetArguments()
 	title, ok := arguments["title"].(string)
 	if !ok || title == "" {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: title parameter is required and must be a string",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: title parameter is required and must be a string"), nil
 	}
 
 	description, ok := arguments["description"].(string)
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: description parameter is required and must be a string",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: description parameter is required and must be a string"), nil
 	}
 
 	reason, ok := arguments["reason"].(string)
 	if !ok || reason == "" {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: reason parameter is required and must be a string",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: reason parameter is required and must be a string"), nil
 	}
 
 	// Change goal
 	if err := s.planner.ChangeGoal(title, description, reason); err != nil {
 		s.logger.Error("Failed to change goal", "error", err)
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: fmt.Sprintf("Error changing goal: %v", err),
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult(fmt.Sprintf("Error changing goal: %v", err)), nil
 	}
 
-	goalText := title
-	if description != "" {
-		goalText = fmt.Sprintf("%s: %s", title, description)
-	}
+	goalText := formatGoalText(title, description)
 	response := fmt.Sprintf("Goal changed to \"%s\" (reason: %s). You probably want to add one or more tasks now.", goalText, reason)
-	return &mcp.CallToolResult{
-		Content: []mcp.Content{
-			mcp.TextContent{
-				Type: "text",
-				Text: response,
-			},
-		},
-	}, nil
+	return createSuccessResult(response), nil
 }
 
 // handleAddTasks handles the project_management__add_tasks tool call
@@ -304,29 +228,13 @@ func (s *Server) handleAddTasks(ctx context.Context, request mcp.CallToolRequest
 	arguments := request.GetArguments()
 	tasksRaw, ok := arguments["tasks"]
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: tasks parameter is required",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: tasks parameter is required"), nil
 	}
 
 	// Convert to slice of interfaces
 	tasksSlice, ok := tasksRaw.([]any)
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: tasks parameter must be an array",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: tasks parameter must be an array"), nil
 	}
 
 	// Parse tasks
@@ -334,28 +242,12 @@ func (s *Server) handleAddTasks(ctx context.Context, request mcp.CallToolRequest
 	for _, taskRaw := range tasksSlice {
 		taskMap, ok := taskRaw.(map[string]any)
 		if !ok {
-			return &mcp.CallToolResult{
-				Content: []mcp.Content{
-					mcp.TextContent{
-						Type: "text",
-						Text: "Error: each task must be an object",
-					},
-				},
-				IsError: true,
-			}, nil
+			return createErrorResult("Error: each task must be an object"), nil
 		}
 
 		title, ok := taskMap["title"].(string)
 		if !ok || title == "" {
-			return &mcp.CallToolResult{
-				Content: []mcp.Content{
-					mcp.TextContent{
-						Type: "text",
-						Text: "Error: each task must have a non-empty title",
-					},
-				},
-				IsError: true,
-			}, nil
+			return createErrorResult("Error: each task must have a non-empty title"), nil
 		}
 
 		description, _ := taskMap["description"].(string)
@@ -370,15 +262,7 @@ func (s *Server) handleAddTasks(ctx context.Context, request mcp.CallToolRequest
 	result, err := s.planner.AddTasks(tasks)
 	if err != nil {
 		s.logger.Error("Failed to add tasks", "error", err)
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: fmt.Sprintf("Error adding tasks: %v", err),
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult(fmt.Sprintf("Error adding tasks: %v", err)), nil
 	}
 
 	// Get the full task list with goal and legend
@@ -397,14 +281,7 @@ func (s *Server) handleAddTasks(ctx context.Context, request mcp.CallToolRequest
 		// Had existing tasks - just show the task list (like get_tasks)
 		response = taskList
 	}
-	return &mcp.CallToolResult{
-		Content: []mcp.Content{
-			mcp.TextContent{
-				Type: "text",
-				Text: response,
-			},
-		},
-	}, nil
+	return createSuccessResult(response), nil
 }
 
 // handleGetTasks handles the project_management__get_tasks tool call
@@ -425,14 +302,7 @@ func (s *Server) handleGetTasks(ctx context.Context, request mcp.CallToolRequest
 		taskList = s.planner.GetTasksByStatus(statusFilter)
 	}
 
-	return &mcp.CallToolResult{
-		Content: []mcp.Content{
-			mcp.TextContent{
-				Type: "text",
-				Text: taskList,
-			},
-		},
-	}, nil
+	return createSuccessResult(taskList), nil
 }
 
 // handleUpdateTaskStatuses handles the project_management__update_task_statuses tool call
@@ -443,41 +313,17 @@ func (s *Server) handleUpdateTaskStatuses(ctx context.Context, request mcp.CallT
 	arguments := request.GetArguments()
 	tasksRaw, ok := arguments["tasks"]
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: tasks parameter is required",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: tasks parameter is required"), nil
 	}
 
 	// Convert to slice of interfaces
 	tasksSlice, ok := tasksRaw.([]any)
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: tasks parameter must be an array",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: tasks parameter must be an array"), nil
 	}
 
 	if len(tasksSlice) == 0 {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: at least one task update is required",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: at least one task update is required"), nil
 	}
 
 	// Parse task updates
@@ -485,41 +331,17 @@ func (s *Server) handleUpdateTaskStatuses(ctx context.Context, request mcp.CallT
 	for _, taskRaw := range tasksSlice {
 		taskMap, ok := taskRaw.(map[string]any)
 		if !ok {
-			return &mcp.CallToolResult{
-				Content: []mcp.Content{
-					mcp.TextContent{
-						Type: "text",
-						Text: "Error: each task update must be an object",
-					},
-				},
-				IsError: true,
-			}, nil
+			return createErrorResult("Error: each task update must be an object"), nil
 		}
 
 		taskID, ok := taskMap["task_id"].(string)
 		if !ok || taskID == "" {
-			return &mcp.CallToolResult{
-				Content: []mcp.Content{
-					mcp.TextContent{
-						Type: "text",
-						Text: "Error: each task update must have a non-empty task_id",
-					},
-				},
-				IsError: true,
-			}, nil
+			return createErrorResult("Error: each task update must have a non-empty task_id"), nil
 		}
 
 		statusStr, ok := taskMap["status"].(string)
 		if !ok || statusStr == "" {
-			return &mcp.CallToolResult{
-				Content: []mcp.Content{
-					mcp.TextContent{
-						Type: "text",
-						Text: "Error: each task update must have a non-empty status",
-					},
-				},
-				IsError: true,
-			}, nil
+			return createErrorResult("Error: each task update must have a non-empty status"), nil
 		}
 
 		// Parse status
@@ -534,27 +356,12 @@ func (s *Server) handleUpdateTaskStatuses(ctx context.Context, request mcp.CallT
 	// Update task statuses
 	if err := s.planner.UpdateTasks(updates); err != nil {
 		s.logger.Error("Failed to update task statuses", "error", err)
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: fmt.Sprintf("Error updating task statuses: %v", err),
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult(fmt.Sprintf("Error updating task statuses: %v", err)), nil
 	}
 
 	// Return full task list
 	taskList := s.planner.GetTasks()
-	return &mcp.CallToolResult{
-		Content: []mcp.Content{
-			mcp.TextContent{
-				Type: "text",
-				Text: taskList,
-			},
-		},
-	}, nil
+	return createSuccessResult(taskList), nil
 }
 
 // handleDeleteTasks handles the project_management__delete_tasks tool call
@@ -565,41 +372,17 @@ func (s *Server) handleDeleteTasks(ctx context.Context, request mcp.CallToolRequ
 	arguments := request.GetArguments()
 	taskIDsRaw, ok := arguments["task_ids"]
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: task_ids parameter is required",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: task_ids parameter is required"), nil
 	}
 
 	// Convert to slice of interfaces
 	taskIDsSlice, ok := taskIDsRaw.([]any)
 	if !ok {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: task_ids parameter must be an array",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: task_ids parameter must be an array"), nil
 	}
 
 	if len(taskIDsSlice) == 0 {
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: "Error: at least one task ID is required",
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult("Error: at least one task ID is required"), nil
 	}
 
 	// Parse task IDs
@@ -607,15 +390,7 @@ func (s *Server) handleDeleteTasks(ctx context.Context, request mcp.CallToolRequ
 	for _, taskIDRaw := range taskIDsSlice {
 		taskID, ok := taskIDRaw.(string)
 		if !ok || taskID == "" {
-			return &mcp.CallToolResult{
-				Content: []mcp.Content{
-					mcp.TextContent{
-						Type: "text",
-						Text: "Error: each task ID must be a non-empty string",
-					},
-				},
-				IsError: true,
-			}, nil
+			return createErrorResult("Error: each task ID must be a non-empty string"), nil
 		}
 		taskIDs = append(taskIDs, taskID)
 	}
@@ -623,27 +398,12 @@ func (s *Server) handleDeleteTasks(ctx context.Context, request mcp.CallToolRequ
 	// Delete tasks
 	if err := s.planner.DeleteTasks(taskIDs); err != nil {
 		s.logger.Error("Failed to delete tasks", "error", err)
-		return &mcp.CallToolResult{
-			Content: []mcp.Content{
-				mcp.TextContent{
-					Type: "text",
-					Text: fmt.Sprintf("Error deleting tasks: %v", err),
-				},
-			},
-			IsError: true,
-		}, nil
+		return createErrorResult(fmt.Sprintf("Error deleting tasks: %v", err)), nil
 	}
 
 	// Return full task list
 	taskList := s.planner.GetTasks()
-	return &mcp.CallToolResult{
-		Content: []mcp.Content{
-			mcp.TextContent{
-				Type: "text",
-				Text: taskList,
-			},
-		},
-	}, nil
+	return createSuccessResult(taskList), nil
 }
 
 // GetServer returns the underlying MCP server