Add Widget-Slot Architecture Skill

apps/mcp-server/package.json

@@ -61,7 +61,7 @@
   "dependencies": {
     "@anthropic-ai/sdk": "0.72.1",
     "@inquirer/prompts": "8.1.0",
-    "@modelcontextprotocol/sdk": "1.25.2",
+    "@modelcontextprotocol/sdk": "1.26.0",
     "@nestjs/common": "11.1.9",
     "@nestjs/config": "4.0.2",
     "@nestjs/core": "11.1.9",

apps/mcp-server/src/mcp/mcp.controller.spec.ts

@@ -2,19 +2,21 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { Response, Request } from 'express';
 
 // Hoist mock variables and class
-const { mockHandlePostMessage, MockSSEServerTransport } = vi.hoisted(() => {
-  const mockHandlePostMessage = vi.fn();
-
-  class MockSSEServerTransport {
-    constructor(
-      public endpoint: string,
-      public response: unknown,
-    ) {}
-    handlePostMessage = mockHandlePostMessage;
-  }
-
-  return { mockHandlePostMessage, MockSSEServerTransport };
-});
+const { mockHandlePostMessage, mockServerClose, MockSSEServerTransport } =
+  vi.hoisted(() => {
+    const mockHandlePostMessage = vi.fn();
+    const mockServerClose = vi.fn().mockResolvedValue(undefined);
+
+    class MockSSEServerTransport {
+      constructor(
+        public endpoint: string,
+        public response: unknown,
+      ) {}
+      handlePostMessage = mockHandlePostMessage;
+    }
+
+    return { mockHandlePostMessage, mockServerClose, MockSSEServerTransport };
+  });
 
 vi.mock('@modelcontextprotocol/sdk/server/sse.js', () => ({
   SSEServerTransport: MockSSEServerTransport,
@@ -24,6 +26,37 @@ vi.mock('@modelcontextprotocol/sdk/server/sse.js', () => ({
 import { McpController } from './mcp.controller';
 import { McpService } from './mcp.service';
 
+/**
+ * Type for accessing private connection property in tests.
+ * Extracted to reduce code duplication in test assertions.
+ */
+type ConnectionAccessor = {
+  connection: {
+    server: unknown;
+    transport: InstanceType<typeof MockSSEServerTransport>;
+  } | null;
+};
+
+/**
+ * Helper to access private connection property from controller.
+ * Uses type assertion to access internal state for testing.
+ *
+ * @param ctrl - McpController instance
+ * @returns The connection object or null
+ */
+const getConnection = (ctrl: McpController): ConnectionAccessor['connection'] =>
+  (ctrl as unknown as ConnectionAccessor).connection;
+
+/**
+ * Helper to create a mock Response object for testing.
+ *
+ * @returns Partial<Response> with status and send mocked
+ */
+const createMockResponse = (): Partial<Response> => ({
+  status: vi.fn().mockReturnThis(),
+  send: vi.fn().mockReturnThis(),
+});
+
 describe('McpController', () => {
   let controller: McpController;
   let mockMcpService: Partial<McpService>;
@@ -38,13 +71,16 @@ describe('McpController', () => {
     mockMcpService = {
       getServer: vi.fn().mockReturnValue({
         connect: mockConnect,
+        close: mockServerClose,
       }),
+      // Return a NEW server instance on each call
+      createServer: vi.fn().mockImplementation(() => ({
+        connect: mockConnect,
+        close: mockServerClose,
+      })),
     };
 
-    mockResponse = {
-      status: vi.fn().mockReturnThis(),
-      send: vi.fn().mockReturnThis(),
-    };
+    mockResponse = createMockResponse();
 
     mockRequest = {
       body: {},
@@ -58,48 +94,92 @@ describe('McpController', () => {
     it('should create SSEServerTransport with correct endpoint', async () => {
       await controller.handleSse(mockResponse as Response);
 
-      // Access private transport via type assertion
-      const transport = (
-        controller as unknown as {
-          transport: InstanceType<typeof MockSSEServerTransport>;
-        }
-      ).transport;
-      expect(transport).toBeInstanceOf(MockSSEServerTransport);
-      expect(transport.endpoint).toBe('/messages');
-      expect(transport.response).toBe(mockResponse);
+      const connection = getConnection(controller);
+      expect(connection).toBeDefined();
+      expect(connection!.transport).toBeInstanceOf(MockSSEServerTransport);
+      expect(connection!.transport.endpoint).toBe('/messages');
+      expect(connection!.transport.response).toBe(mockResponse);
     });
 
     it('should connect transport to MCP server', async () => {
       await controller.handleSse(mockResponse as Response);
 
-      expect(mockMcpService.getServer).toHaveBeenCalled();
+      expect(mockMcpService.createServer).toHaveBeenCalled();
       expect(mockConnect).toHaveBeenCalled();
     });
 
     it('should replace existing transport on new connection', async () => {
       // First connection
       await controller.handleSse(mockResponse as Response);
-      const firstTransport = (
-        controller as unknown as {
-          transport: InstanceType<typeof MockSSEServerTransport>;
-        }
-      ).transport;
+      const firstConnection = getConnection(controller);
 
       // Second connection (should replace)
-      const newMockResponse: Partial<Response> = {
-        status: vi.fn().mockReturnThis(),
-        send: vi.fn().mockReturnThis(),
-      };
+      const newMockResponse = createMockResponse();
+      await controller.handleSse(newMockResponse as Response);
+
+      const secondConnection = getConnection(controller);
+
+      expect(secondConnection!.transport).not.toBe(firstConnection!.transport);
+      expect(secondConnection!.transport.response).toBe(newMockResponse);
+    });
+
+    it('should close previous server before creating new connection', async () => {
+      // First connection
+      await controller.handleSse(mockResponse as Response);
+
+      // Second connection - should close previous server first
+      const newMockResponse = createMockResponse();
       await controller.handleSse(newMockResponse as Response);
 
-      const secondTransport = (
-        controller as unknown as {
-          transport: InstanceType<typeof MockSSEServerTransport>;
-        }
-      ).transport;
+      // server.close() should have been called once (for the first connection)
+      expect(mockServerClose).toHaveBeenCalledTimes(1);
+    });
+
+    it('should handle server.close() error gracefully', async () => {
+      // Make close throw an error
+      mockServerClose.mockRejectedValueOnce(new Error('Close failed'));
 
-      expect(secondTransport).not.toBe(firstTransport);
-      expect(secondTransport.response).toBe(newMockResponse);
+      // First connection
+      await controller.handleSse(mockResponse as Response);
+
+      // Second connection - should handle close error gracefully
+      const newMockResponse = createMockResponse();
+
+      // Should not throw even if close fails
+      await expect(
+        controller.handleSse(newMockResponse as Response),
+      ).resolves.not.toThrow();
+
+      // New connection should still be established
+      expect(mockMcpService.createServer).toHaveBeenCalledTimes(2);
+    });
+
+    it('should return 500 when server creation fails', async () => {
+      // Make createServer throw an error
+      (
+        mockMcpService.createServer as ReturnType<typeof vi.fn>
+      ).mockImplementationOnce(() => {
+        throw new Error('Server creation failed');
+      });
+
+      await controller.handleSse(mockResponse as Response);
+
+      expect(mockResponse.status).toHaveBeenCalledWith(500);
+      expect(mockResponse.send).toHaveBeenCalledWith(
+        'Failed to establish SSE connection',
+      );
+    });
+
+    it('should return 500 when server.connect fails', async () => {
+      // Make connect throw an error
+      mockConnect.mockRejectedValueOnce(new Error('Connection failed'));
+
+      await controller.handleSse(mockResponse as Response);
+
+      expect(mockResponse.status).toHaveBeenCalledWith(500);
+      expect(mockResponse.send).toHaveBeenCalledWith(
+        'Failed to establish SSE connection',
+      );
     });
   });
 
@@ -126,10 +206,7 @@ describe('McpController', () => {
         body: { method: 'tools/list' },
         headers: {},
       };
-      const messageResponse: Partial<Response> = {
-        status: vi.fn().mockReturnThis(),
-        send: vi.fn().mockReturnThis(),
-      };
+      const messageResponse = createMockResponse();
 
       await controller.handleMessages(
         messageRequest as Request,
@@ -147,10 +224,7 @@ describe('McpController', () => {
       await controller.handleSse(mockResponse as Response);
 
       // Reconnect
-      const newSseResponse: Partial<Response> = {
-        status: vi.fn().mockReturnThis(),
-        send: vi.fn().mockReturnThis(),
-      };
+      const newSseResponse = createMockResponse();
       await controller.handleSse(newSseResponse as Response);
 
       // Send message - should use the new transport
@@ -176,4 +250,80 @@ describe('McpController', () => {
       expect(mockHandlePostMessage).not.toHaveBeenCalled();
     });
   });
+
+  describe('Security: Server instance isolation', () => {
+    it('should NOT reuse the same Server instance for multiple connections', async () => {
+      // This test verifies the security fix: each connection gets its own Server instance
+
+      // First connection
+      await controller.handleSse(mockResponse as Response);
+      const firstServer = (
+        mockMcpService.createServer as ReturnType<typeof vi.fn>
+      ).mock.results[0].value;
+
+      // Second connection
+      const newMockResponse = createMockResponse();
+      await controller.handleSse(newMockResponse as Response);
+      const secondServer = (
+        mockMcpService.createServer as ReturnType<typeof vi.fn>
+      ).mock.results[1].value;
+
+      // After fix: servers should be DIFFERENT instances (per-connection)
+      expect(secondServer).not.toBe(firstServer);
+
+      // Both connections should have called createServer()
+      expect(mockMcpService.createServer).toHaveBeenCalledTimes(2);
+    });
+
+    it('should isolate transport instances between connections', async () => {
+      // First connection
+      await controller.handleSse(mockResponse as Response);
+      const firstConnection = getConnection(controller);
+
+      // Second connection
+      const newMockResponse = createMockResponse();
+      await controller.handleSse(newMockResponse as Response);
+      const secondConnection = getConnection(controller);
+
+      // Transports should be different instances
+      expect(secondConnection!.transport).not.toBe(firstConnection!.transport);
+      expect(secondConnection!.transport.response).toBe(newMockResponse);
+    });
+  });
+
+  describe('Resource cleanup', () => {
+    it('should close connection on module destroy', async () => {
+      // Establish connection
+      await controller.handleSse(mockResponse as Response);
+
+      // Trigger module destroy
+      await controller.onModuleDestroy();
+
+      // Server should be closed
+      expect(mockServerClose).toHaveBeenCalledTimes(1);
+
+      // Connection should be null
+      const connection = getConnection(controller);
+      expect(connection).toBeNull();
+    });
+
+    it('should handle module destroy when no connection exists', async () => {
+      // No connection established, should not throw
+      await expect(controller.onModuleDestroy()).resolves.not.toThrow();
+
+      // close should not have been called
+      expect(mockServerClose).not.toHaveBeenCalled();
+    });
+
+    it('should handle close error on module destroy gracefully', async () => {
+      // Establish connection
+      await controller.handleSse(mockResponse as Response);
+
+      // Make close throw an error
+      mockServerClose.mockRejectedValueOnce(new Error('Close failed'));
+
+      // Should not throw
+      await expect(controller.onModuleDestroy()).resolves.not.toThrow();
+    });
+  });
 });