test: add mock-based tests for MCPAgent

Implement unit tests for MCPAgent using a mocked MCP client.\n\n- Define an mcpClient interface and a mock implementation in the test file.\n- Modify MCPAgent to use the interface and add an exported ClientOverride field\n  to allow injecting the mock during tests.\n- Export necessary constants and argument structs from the agent package.\n- Add test cases covering success, tool errors, empty responses, and pipe errors\n  for both GetArtistBiography and GetArtistURL.\n- Fix agent logic to handle empty TextContent in responses correctly.\n- Remove previous placeholder tests and unreliable initialization test.

core/agents/mcp/mcp_agent.go

@@ -19,43 +19,65 @@ import (
 	"github.com/navidrome/navidrome/model"
 )
 
+// Exported constants for testing
 const (
-	mcpAgentName          = "mcp"
+	McpAgentName          = "mcp"
-	mcpServerPath         = "/Users/deluan/Development/navidrome/plugins-mcp/mcp-server"
+	McpServerPath         = "/Users/deluan/Development/navidrome/plugins-mcp/mcp-server"
-	mcpToolNameGetBio     = "get_artist_biography"
+	McpToolNameGetBio     = "get_artist_biography"
-	mcpToolNameGetURL     = "get_artist_url"
+	McpToolNameGetURL     = "get_artist_url"
 	initializationTimeout = 10 * time.Second
 )
 
+// mcpClient interface matching the methods used from mcp.Client
+// Allows for mocking in tests.
+type mcpClient interface {
+	Initialize(ctx context.Context) (*mcp.InitializeResponse, error)
+	CallTool(ctx context.Context, toolName string, args any) (*mcp.ToolResponse, error)
+}
+
 // MCPAgent interacts with an external MCP server for metadata retrieval.
 // It keeps a single instance of the server process running and attempts restart on failure.
 type MCPAgent struct {
 	mu sync.Mutex
-	// No longer using sync.Once to allow re-initialization
-	// initErr error // Error state managed implicitly by client being nil
 
 	cmd    *exec.Cmd
 	stdin  io.WriteCloser
-	client *mcp.Client
+	client mcpClient // Use the interface type here
+
+	// ClientOverride allows injecting a mock client for testing.
+	// This field should ONLY be set in test code.
+	ClientOverride mcpClient
 }
 
 func mcpConstructor(ds model.DataStore) agents.Interface {
 	// Check if the MCP server executable exists
-	if _, err := os.Stat(mcpServerPath); os.IsNotExist(err) {
+	if _, err := os.Stat(McpServerPath); os.IsNotExist(err) { // Use exported constant
-		log.Warn("MCP server executable not found, disabling agent", "path", mcpServerPath, "error", err)
+		log.Warn("MCP server executable not found, disabling agent", "path", McpServerPath, "error", err)
 		return nil
 	}
-	log.Info("MCP Agent created, server will be started on first request", "serverPath", mcpServerPath)
+	log.Info("MCP Agent created, server will be started on first request", "serverPath", McpServerPath)
 	return &MCPAgent{}
 }
 
 func (a *MCPAgent) AgentName() string {
-	return mcpAgentName
+	return McpAgentName // Use exported constant
 }
 
 // ensureClientInitialized starts the MCP server process and initializes the client if needed.
 // It now attempts restart if the client is found to be nil.
 func (a *MCPAgent) ensureClientInitialized(ctx context.Context) (err error) {
+	// --- Use override if provided (for testing) ---
+	if a.ClientOverride != nil {
+		a.mu.Lock()
+		// Only set if not already set (could be set by a concurrent test setup)
+		if a.client == nil {
+			a.client = a.ClientOverride
+			log.Debug(ctx, "Using provided MCP client override for testing")
+		}
+		a.mu.Unlock()
+		return nil // Skip real initialization when override is present
+	}
+
 	a.mu.Lock()
 	// If client is already initialized and valid, we're done.
 	if a.client != nil {
@@ -66,10 +88,10 @@ func (a *MCPAgent) ensureClientInitialized(ctx context.Context) (err error) {
 	a.mu.Unlock()
 
 	// --- Attempt initialization/restart ---
-	log.Info(ctx, "Initializing MCP client and starting/restarting server process...", "serverPath", mcpServerPath)
+	log.Info(ctx, "Initializing MCP client and starting/restarting server process...", "serverPath", McpServerPath)
 
 	// Use background context for the command itself, so it doesn't get cancelled by the request context.
-	cmd := exec.CommandContext(context.Background(), mcpServerPath)
+	cmd := exec.CommandContext(context.Background(), McpServerPath)
 
 	var stdin io.WriteCloser
 	var stdout io.ReadCloser
@@ -127,11 +149,12 @@ func (a *MCPAgent) ensureClientInitialized(ctx context.Context) (err error) {
 
 	// --- Initialize MCP client ---
 	transport := stdio.NewStdioServerTransportWithIO(stdout, stdin) // Use the pipes from this attempt
-	client := mcp.NewClient(transport)
+	// Create the *real* mcp.Client, which satisfies our mcpClient interface
+	clientImpl := mcp.NewClient(transport)
 
 	initCtx, cancel := context.WithTimeout(context.Background(), initializationTimeout)
 	defer cancel()
-	if _, err = client.Initialize(initCtx); err != nil {
+	if _, err = clientImpl.Initialize(initCtx); err != nil {
 		err = fmt.Errorf("failed to initialize MCP client: %w", err)
 		log.Error(ctx, "MCP client initialization failed after process start", "pid", currentPid, "error", err)
 		// Attempt to kill the process we just started, as client init failed
@@ -155,17 +178,18 @@ func (a *MCPAgent) ensureClientInitialized(ctx context.Context) (err error) {
 		return nil // Return success as *a* client is available
 	}
 
-	a.cmd = cmd       // Store the successfully started command
+	a.cmd = cmd           // Store the successfully started command
-	a.stdin = stdin   // Store its stdin
+	a.stdin = stdin       // Store its stdin
-	a.client = client // Store the successfully initialized client
+	a.client = clientImpl // Store the successfully initialized client (as interface type)
 	a.mu.Unlock()
 
 	log.Info(ctx, "MCP client initialized successfully", "pid", currentPid)
 	return nil // Success
 }
 
-// getArtistBiographyArgs defines the structure for the get_artist_biography MCP tool arguments.
+// GetArtistBiographyArgs defines the structure for the get_artist_biography MCP tool arguments.
-type getArtistBiographyArgs struct {
+// Exported for use in tests.
+type GetArtistBiographyArgs struct {
 	ID   string `json:"id"`
 	Name string `json:"name"`
 	Mbid string `json:"mbid,omitempty"`
@@ -197,42 +221,43 @@ func (a *MCPAgent) GetArtistBiography(ctx context.Context, id, name, mbid string
 	log.Debug(ctx, "Calling MCP agent GetArtistBiography", "id", id, "name", name, "mbid", mbid)
 
 	// Prepare arguments for the tool call
-	args := getArtistBiographyArgs{
+	args := GetArtistBiographyArgs{
 		ID:   id,
 		Name: name,
 		Mbid: mbid,
 	}
 
 	// Call the tool using the client reference
-	log.Debug(ctx, "Calling MCP tool", "tool", mcpToolNameGetBio, "args", args)
+	log.Debug(ctx, "Calling MCP tool", "tool", McpToolNameGetBio, "args", args)
-	response, err := currentClient.CallTool(ctx, mcpToolNameGetBio, args) // Use currentClient
+	response, err := currentClient.CallTool(ctx, McpToolNameGetBio, args) // Use currentClient
 	if err != nil {
 		// Handle potential pipe closures or other communication errors
-		log.Error(ctx, "Failed to call MCP tool", "tool", mcpToolNameGetBio, "error", err)
+		log.Error(ctx, "Failed to call MCP tool", "tool", McpToolNameGetBio, "error", err)
 		// Check if the error indicates a broken pipe, suggesting the server died
 		if errors.Is(err, io.ErrClosedPipe) || strings.Contains(err.Error(), "broken pipe") || strings.Contains(err.Error(), "EOF") {
 			log.Warn(ctx, "MCP tool call failed, possibly due to server process exit. State will be reset.")
 			// State reset is handled by the monitoring goroutine, just return error
 			return "", fmt.Errorf("MCP agent process communication error: %w", err)
 		}
-		return "", fmt.Errorf("failed to call MCP tool '%s': %w", mcpToolNameGetBio, err)
+		return "", fmt.Errorf("failed to call MCP tool '%s': %w", McpToolNameGetBio, err)
 	}
 
 	// Process the response
-	if response == nil || len(response.Content) == 0 || response.Content[0].TextContent == nil {
+	if response == nil || len(response.Content) == 0 || response.Content[0].TextContent == nil || response.Content[0].TextContent.Text == "" {
-		log.Warn(ctx, "MCP tool returned empty or invalid response", "tool", mcpToolNameGetBio)
+		log.Warn(ctx, "MCP tool returned empty or invalid response", "tool", McpToolNameGetBio)
 		return "", agents.ErrNotFound
 	}
 
 	bio := response.Content[0].TextContent.Text
-	log.Debug(ctx, "Received biography from MCP agent", "tool", mcpToolNameGetBio, "bioLength", len(bio))
+	log.Debug(ctx, "Received biography from MCP agent", "tool", McpToolNameGetBio, "bioLength", len(bio))
 
 	// Return the biography text
 	return bio, nil
 }
 
-// getArtistURLArgs defines the structure for the get_artist_url MCP tool arguments.
+// GetArtistURLArgs defines the structure for the get_artist_url MCP tool arguments.
-type getArtistURLArgs struct {
+// Exported for use in tests.
+type GetArtistURLArgs struct {
 	ID   string `json:"id"`
 	Name string `json:"name"`
 	Mbid string `json:"mbid,omitempty"`
@@ -263,35 +288,35 @@ func (a *MCPAgent) GetArtistURL(ctx context.Context, id, name, mbid string) (str
 	log.Debug(ctx, "Calling MCP agent GetArtistURL", "id", id, "name", name, "mbid", mbid)
 
 	// Prepare arguments for the tool call
-	args := getArtistURLArgs{
+	args := GetArtistURLArgs{
 		ID:   id,
 		Name: name,
 		Mbid: mbid,
 	}
 
 	// Call the tool using the client reference
-	log.Debug(ctx, "Calling MCP tool", "tool", mcpToolNameGetURL, "args", args)
+	log.Debug(ctx, "Calling MCP tool", "tool", McpToolNameGetURL, "args", args)
-	response, err := currentClient.CallTool(ctx, mcpToolNameGetURL, args) // Use currentClient
+	response, err := currentClient.CallTool(ctx, McpToolNameGetURL, args) // Use currentClient
 	if err != nil {
 		// Handle potential pipe closures or other communication errors
-		log.Error(ctx, "Failed to call MCP tool", "tool", mcpToolNameGetURL, "error", err)
+		log.Error(ctx, "Failed to call MCP tool", "tool", McpToolNameGetURL, "error", err)
 		// Check if the error indicates a broken pipe, suggesting the server died
 		if errors.Is(err, io.ErrClosedPipe) || strings.Contains(err.Error(), "broken pipe") || strings.Contains(err.Error(), "EOF") {
 			log.Warn(ctx, "MCP tool call failed, possibly due to server process exit. State will be reset.")
 			// State reset is handled by the monitoring goroutine, just return error
 			return "", fmt.Errorf("MCP agent process communication error: %w", err)
 		}
-		return "", fmt.Errorf("failed to call MCP tool '%s': %w", mcpToolNameGetURL, err)
+		return "", fmt.Errorf("failed to call MCP tool '%s': %w", McpToolNameGetURL, err)
 	}
 
 	// Process the response
-	if response == nil || len(response.Content) == 0 || response.Content[0].TextContent == nil {
+	if response == nil || len(response.Content) == 0 || response.Content[0].TextContent == nil || response.Content[0].TextContent.Text == "" {
-		log.Warn(ctx, "MCP tool returned empty or invalid response", "tool", mcpToolNameGetURL)
+		log.Warn(ctx, "MCP tool returned empty or invalid response", "tool", McpToolNameGetURL)
 		return "", agents.ErrNotFound
 	}
 
 	url := response.Content[0].TextContent.Text
-	log.Debug(ctx, "Received URL from MCP agent", "tool", mcpToolNameGetURL, "url", url)
+	log.Debug(ctx, "Received URL from MCP agent", "tool", McpToolNameGetURL, "url", url)
 
 	// Return the URL text
 	return url, nil
@@ -302,6 +327,6 @@ var _ agents.ArtistBiographyRetriever = (*MCPAgent)(nil)
 var _ agents.ArtistURLRetriever = (*MCPAgent)(nil)
 
 func init() {
-	agents.Register(mcpAgentName, mcpConstructor)
+	agents.Register(McpAgentName, mcpConstructor)
 	log.Info("Registered MCP Agent")
 }

core/agents/mcp/mcp_agent_test.go

@@ -2,54 +2,188 @@ package mcp_test
 
 import (
 	"context"
+	"errors"
+	"fmt"
+	"io"
 
+	mcp_client "github.com/metoro-io/mcp-golang" // Renamed alias for clarity
 	"github.com/navidrome/navidrome/core/agents"
 	"github.com/navidrome/navidrome/core/agents/mcp"
-	"github.com/navidrome/navidrome/tests"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
+// mcpClient defines the interface for the MCP client methods used by the agent.
+// This allows mocking the client for testing.
+type mcpClient interface {
+	Initialize(ctx context.Context) (*mcp_client.InitializeResponse, error)
+	CallTool(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error)
+}
+
+// mockMCPClient is a mock implementation of mcpClient for testing.
+type mockMCPClient struct {
+	InitializeFunc func(ctx context.Context) (*mcp_client.InitializeResponse, error)
+	CallToolFunc   func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error)
+	callToolArgs   []any  // Store args for verification
+	callToolName   string // Store tool name for verification
+}
+
+func (m *mockMCPClient) Initialize(ctx context.Context) (*mcp_client.InitializeResponse, error) {
+	if m.InitializeFunc != nil {
+		return m.InitializeFunc(ctx)
+	}
+	return &mcp_client.InitializeResponse{}, nil // Default success
+}
+
+func (m *mockMCPClient) CallTool(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+	m.callToolName = toolName
+	m.callToolArgs = append(m.callToolArgs, args)
+	if m.CallToolFunc != nil {
+		return m.CallToolFunc(ctx, toolName, args)
+	}
+	return &mcp_client.ToolResponse{}, nil
+}
+
+// Ensure mock implements the interface (compile-time check)
+var _ mcpClient = (*mockMCPClient)(nil)
+
 var _ = Describe("MCPAgent", func() {
 	var (
-		ctx context.Context
+		ctx        context.Context
-		// ds    model.DataStore // Not needed yet for PoC
+		agent      *mcp.MCPAgent // Use concrete type from the package
-		agent agents.ArtistBiographyRetriever
+		mockClient *mockMCPClient
 	)
 
 	BeforeEach(func() {
 		ctx = context.Background()
-		// Use ctx to avoid unused variable error
+		mockClient = &mockMCPClient{
-		_ = ctx
+			callToolArgs: make([]any, 0), // Reset args on each test
-		ds := &tests.MockDataStore{}
+		}
-		// Use ds to avoid unused variable error
-		_ = ds
-		// Directly instantiate for now, assuming constructor logic is minimal for PoC
-		// In a real scenario, you might use the constructor or mock dependencies
 
-		// The constructor is not exported, we need to access it differently or make it testable.
+		// Instantiate the real agent
-		// For PoC, let's assume we can get an instance. We might need to adjust mcp_agent.go later
-		// For now, comment out the direct constructor call for simplicity in test setup phase.
-		// constructor := mcp.mcpConstructor // This won't work as it's unexported
-
-		// Placeholder: Create a simple MCPAgent instance directly for testing its existence.
-		// This bypasses the constructor logic (like the file check), which is fine for a basic test.
 		agent = &mcp.MCPAgent{}
-
+		// Inject the mock client directly using the exported override field
-		Expect(agent).NotTo(BeNil())
+		agent.ClientOverride = mockClient
 	})
 
-	It("should be created", func() {
+	Describe("GetArtistBiography", func() {
-		Expect(agent).NotTo(BeNil())
+		It("should call the correct tool and return the biography", func() {
+			expectedBio := "This is the artist bio."
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				Expect(toolName).To(Equal(mcp.McpToolNameGetBio))
+				Expect(args).To(BeAssignableToTypeOf(mcp.GetArtistBiographyArgs{})) // Use exported type
+				typedArgs := args.(mcp.GetArtistBiographyArgs)                      // Use exported type
+				Expect(typedArgs.ID).To(Equal("id1"))
+				Expect(typedArgs.Name).To(Equal("Artist Name"))
+				Expect(typedArgs.Mbid).To(Equal("mbid1"))
+				return mcp_client.NewToolResponse(mcp_client.NewTextContent(expectedBio)), nil
+			}
+
+			bio, err := agent.GetArtistBiography(ctx, "id1", "Artist Name", "mbid1")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(bio).To(Equal(expectedBio))
+		})
+
+		It("should return error if CallTool fails", func() {
+			expectedErr := errors.New("mcp tool error")
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				return nil, expectedErr
+			}
+
+			bio, err := agent.GetArtistBiography(ctx, "id1", "Artist Name", "mbid1")
+			Expect(err).To(HaveOccurred())
+			Expect(errors.Is(err, expectedErr)).To(BeTrue())
+			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("failed to call MCP tool '%s'", mcp.McpToolNameGetBio)))
+			Expect(bio).To(BeEmpty())
+		})
+
+		It("should return ErrNotFound if CallTool response is empty", func() {
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				// Return a response created with no content parts
+				return mcp_client.NewToolResponse(), nil
+			}
+
+			bio, err := agent.GetArtistBiography(ctx, "id1", "Artist Name", "mbid1")
+			Expect(err).To(MatchError(agents.ErrNotFound))
+			Expect(bio).To(BeEmpty())
+		})
+
+		It("should return ErrNotFound if CallTool response has nil TextContent (simulated by empty string)", func() {
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				// Simulate nil/empty text content by creating response with empty string text
+				return mcp_client.NewToolResponse(mcp_client.NewTextContent("")), nil
+			}
+
+			bio, err := agent.GetArtistBiography(ctx, "id1", "Artist Name", "mbid1")
+			Expect(err).To(MatchError(agents.ErrNotFound))
+			Expect(bio).To(BeEmpty())
+		})
+
+		It("should return comm error if CallTool returns pipe error", func() {
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				return nil, io.ErrClosedPipe
+			}
+
+			bio, err := agent.GetArtistBiography(ctx, "id1", "Artist Name", "mbid1")
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("MCP agent process communication error"))
+			Expect(errors.Is(err, io.ErrClosedPipe)).To(BeTrue())
+			Expect(bio).To(BeEmpty())
+		})
 	})
 
-	// TODO: Add PoC test case that calls GetArtistBiography
+	Describe("GetArtistURL", func() {
-	// This will likely require the actual MCP server to be running
+		It("should call the correct tool and return the URL", func() {
-	// or mocking the exec.Command part.
+			expectedURL := "http://example.com/artist"
-	It("should call GetArtistBiography (placeholder)", func() {
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
-		// bio, err := agent.GetArtistBiography(ctx, "artist-id", "Artist Name", "mbid-123")
+				Expect(toolName).To(Equal(mcp.McpToolNameGetURL))
-		// Expect(err).ToNot(HaveOccurred())
+				Expect(args).To(BeAssignableToTypeOf(mcp.GetArtistURLArgs{})) // Use exported type
-		// Expect(bio).ToNot(BeEmpty())
+				typedArgs := args.(mcp.GetArtistURLArgs)                      // Use exported type
-		Skip("Skipping actual MCP call for initial PoC test setup")
+				Expect(typedArgs.ID).To(Equal("id2"))
+				Expect(typedArgs.Name).To(Equal("Another Artist"))
+				Expect(typedArgs.Mbid).To(Equal("mbid2"))
+				return mcp_client.NewToolResponse(mcp_client.NewTextContent(expectedURL)), nil
+			}
+
+			url, err := agent.GetArtistURL(ctx, "id2", "Another Artist", "mbid2")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(url).To(Equal(expectedURL))
+		})
+
+		It("should return error if CallTool fails", func() {
+			expectedErr := errors.New("mcp tool error url")
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				return nil, expectedErr
+			}
+
+			url, err := agent.GetArtistURL(ctx, "id2", "Another Artist", "mbid2")
+			Expect(err).To(HaveOccurred())
+			Expect(errors.Is(err, expectedErr)).To(BeTrue())
+			Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("failed to call MCP tool '%s'", mcp.McpToolNameGetURL)))
+			Expect(url).To(BeEmpty())
+		})
+
+		It("should return ErrNotFound if CallTool response is empty", func() {
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				// Return a response created with no content parts
+				return mcp_client.NewToolResponse(), nil
+			}
+
+			url, err := agent.GetArtistURL(ctx, "id2", "Another Artist", "mbid2")
+			Expect(err).To(MatchError(agents.ErrNotFound))
+			Expect(url).To(BeEmpty())
+		})
+
+		It("should return comm error if CallTool returns pipe error", func() {
+			mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) {
+				return nil, fmt.Errorf("write: %w", io.ErrClosedPipe)
+			}
+
+			url, err := agent.GetArtistURL(ctx, "id2", "Another Artist", "mbid2")
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("MCP agent process communication error"))
+			Expect(errors.Is(err, io.ErrClosedPipe)).To(BeTrue())
+			Expect(url).To(BeEmpty())
+		})
 	})
 })