From be9e10db373a6d116817b15cb9df267b9a56c9b1 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 19 Apr 2025 12:56:22 -0400 Subject: [PATCH] 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 | 103 ++++++++++------ core/agents/mcp/mcp_agent_test.go | 194 +++++++++++++++++++++++++----- 2 files changed, 228 insertions(+), 69 deletions(-) diff --git a/core/agents/mcp/mcp_agent.go b/core/agents/mcp/mcp_agent.go index cfac68f05..fb9d8f9b5 100644 --- a/core/agents/mcp/mcp_agent.go +++ b/core/agents/mcp/mcp_agent.go @@ -19,43 +19,65 @@ import ( "github.com/navidrome/navidrome/model" ) +// Exported constants for testing const ( - mcpAgentName = "mcp" - mcpServerPath = "/Users/deluan/Development/navidrome/plugins-mcp/mcp-server" - mcpToolNameGetBio = "get_artist_biography" - mcpToolNameGetURL = "get_artist_url" + McpAgentName = "mcp" + McpServerPath = "/Users/deluan/Development/navidrome/plugins-mcp/mcp-server" + McpToolNameGetBio = "get_artist_biography" + 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) { - log.Warn("MCP server executable not found, disabling agent", "path", mcpServerPath, "error", 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) 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.stdin = stdin // Store its stdin - a.client = client // Store the successfully initialized client + a.cmd = cmd // Store the successfully started command + a.stdin = stdin // Store its stdin + 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. -type getArtistBiographyArgs struct { +// GetArtistBiographyArgs defines the structure for the get_artist_biography MCP tool arguments. +// 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) - response, err := currentClient.CallTool(ctx, mcpToolNameGetBio, args) // Use currentClient + log.Debug(ctx, "Calling MCP tool", "tool", McpToolNameGetBio, "args", args) + 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 { - log.Warn(ctx, "MCP tool returned empty or invalid response", "tool", mcpToolNameGetBio) + 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) 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. -type getArtistURLArgs struct { +// GetArtistURLArgs defines the structure for the get_artist_url MCP tool arguments. +// 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) - response, err := currentClient.CallTool(ctx, mcpToolNameGetURL, args) // Use currentClient + log.Debug(ctx, "Calling MCP tool", "tool", McpToolNameGetURL, "args", args) + 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 { - log.Warn(ctx, "MCP tool returned empty or invalid response", "tool", mcpToolNameGetURL) + 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) 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") } diff --git a/core/agents/mcp/mcp_agent_test.go b/core/agents/mcp/mcp_agent_test.go index e6485da87..803473124 100644 --- a/core/agents/mcp/mcp_agent_test.go +++ b/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 - // ds model.DataStore // Not needed yet for PoC - agent agents.ArtistBiographyRetriever + ctx context.Context + agent *mcp.MCPAgent // Use concrete type from the package + mockClient *mockMCPClient ) BeforeEach(func() { ctx = context.Background() - // Use ctx to avoid unused variable error - _ = ctx - 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 + mockClient = &mockMCPClient{ + callToolArgs: make([]any, 0), // Reset args on each test + } - // The constructor is not exported, we need to access it differently or make it testable. - // 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. + // Instantiate the real agent agent = &mcp.MCPAgent{} - - Expect(agent).NotTo(BeNil()) + // Inject the mock client directly using the exported override field + agent.ClientOverride = mockClient }) - It("should be created", func() { - Expect(agent).NotTo(BeNil()) + Describe("GetArtistBiography", func() { + 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 - // This will likely require the actual MCP server to be running - // or mocking the exec.Command part. - It("should call GetArtistBiography (placeholder)", func() { - // bio, err := agent.GetArtistBiography(ctx, "artist-id", "Artist Name", "mbid-123") - // Expect(err).ToNot(HaveOccurred()) - // Expect(bio).ToNot(BeEmpty()) - Skip("Skipping actual MCP call for initial PoC test setup") + Describe("GetArtistURL", func() { + It("should call the correct tool and return the URL", func() { + expectedURL := "http://example.com/artist" + mockClient.CallToolFunc = func(ctx context.Context, toolName string, args any) (*mcp_client.ToolResponse, error) { + Expect(toolName).To(Equal(mcp.McpToolNameGetURL)) + Expect(args).To(BeAssignableToTypeOf(mcp.GetArtistURLArgs{})) // Use exported type + typedArgs := args.(mcp.GetArtistURLArgs) // Use exported type + 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()) + }) }) })