Merge pull request 'P3 — Typed SPA chat (Phase P webUI)' (#1151) from feat/webui-p3-chat into next

apps/gateway/src/commands/command-executor-p8012.spec.ts

@@ -1,3 +1,4 @@
+import { Logger } from '@nestjs/common';
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { CommandExecutorService } from './command-executor.service.js';
 import type { SlashCommandPayload } from '@mosaicstack/types';
@@ -12,6 +13,7 @@ const mockRegistry = {
       { name: 'agent', aliases: ['a'], scope: 'agent', execution: 'socket', available: true },
       { name: 'prdy', aliases: [], scope: 'agent', execution: 'socket', available: true },
       { name: 'tools', aliases: [], scope: 'agent', execution: 'socket', available: true },
+      { name: 'mcp', aliases: [], scope: 'agent', execution: 'socket', available: true },
     ],
     skills: [],
   })),
@@ -72,7 +74,14 @@ const mockChatGateway = {
   broadcastSessionInfo: vi.fn(),
 };
 
-function buildService(redis: typeof mockRedis | null = mockRedis): CommandExecutorService {
+function buildService(
+  redis: typeof mockRedis | null = mockRedis,
+  mcpClient: {
+    reconnectServer: ReturnType<typeof vi.fn>;
+    getServerStatuses: ReturnType<typeof vi.fn>;
+    getToolDefinitions: ReturnType<typeof vi.fn>;
+  } | null = null,
+): CommandExecutorService {
   return new CommandExecutorService(
     mockRegistry as never,
     mockAgentService as never,
@@ -82,7 +91,7 @@ function buildService(redis: typeof mockRedis | null = mockRedis): CommandExecut
     mockBrain as never,
     null,
     mockChatGateway as never,
-    null,
+    mcpClient as never,
   );
 }
 
@@ -258,4 +267,124 @@ describe('CommandExecutorService — P8-012 commands', () => {
     expect(result.command).toBe('tools');
     expect(result.message).toContain('tools');
   });
+
+  // Top-level catch sanitization (P3-4 re-review finding #1): a rejected
+  // Redis `set` inside /provider login is the only reachable path into the
+  // top-level catch in `execute()`. The raw exception must be logged
+  // server-side but never handed back to the socket client.
+  it('sanitizes the top-level command catch, logging the raw exception but never returning it to the client', async () => {
+    const distinctiveRawFailure = 'ECONNREFUSED distinctive-raw-redis-failure-token-9f31';
+    const rawError = new Error(distinctiveRawFailure);
+    const failingRedis = {
+      set: vi.fn().mockRejectedValue(rawError),
+      get: vi.fn(),
+      del: vi.fn(),
+    };
+    const failingService = buildService(failingRedis as unknown as typeof mockRedis);
+    const loggerErrorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+
+    const payload: SlashCommandPayload = {
+      command: 'provider',
+      args: 'login anthropic',
+      conversationId,
+    };
+    const result = await failingService.execute(payload, userScope);
+
+    expect(result.success).toBe(false);
+    expect(result.command).toBe('provider');
+    expect(result.message).toBe('Command failed due to an internal error.');
+    expect(result.message).not.toContain(distinctiveRawFailure);
+    expect(result.message).not.toContain('ECONNREFUSED');
+
+    // The real exception is still logged server-side, as the raw Error
+    // object itself (not stringified/interpolated into the log message).
+    expect(loggerErrorSpy).toHaveBeenCalled();
+    const loggedRawError = loggerErrorSpy.mock.calls.some((call) => call.includes(rawError));
+    expect(loggedRawError).toBe(true);
+
+    loggerErrorSpy.mockRestore();
+  });
+
+  // Inner catch sanitization (P3-5 operator ruling): every catch in
+  // command-executor.service.ts that returns a SlashCommandResultPayload
+  // must sanitize the client-facing message the same way the top-level
+  // catch does, while still logging the raw exception server-side.
+  it('/agent new sanitizes agent-creation failures, logging the raw exception but never returning it to the client', async () => {
+    const marker = new Error('distinctive-agent-create-failure-token-A17f');
+    mockBrain.agents.create.mockRejectedValueOnce(marker);
+    const loggerErrorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+
+    const payload: SlashCommandPayload = {
+      command: 'agent',
+      args: 'new my-new-agent',
+      conversationId,
+    };
+    const result = await service.execute(payload, userScope);
+
+    expect(result.success).toBe(false);
+    expect(result.command).toBe('agent');
+    expect(result.message).toBe('Failed to create agent due to an internal error.');
+    expect(result.message).not.toContain('distinctive-agent-create-failure-token-A17f');
+
+    expect(loggerErrorSpy).toHaveBeenCalled();
+    const loggedRawError = loggerErrorSpy.mock.calls.some((call) => call.includes(marker));
+    expect(loggedRawError).toBe(true);
+
+    loggerErrorSpy.mockRestore();
+  });
+
+  it('/agent <name> switch sanitizes agent-lookup failures, logging the raw exception but never returning it to the client', async () => {
+    const marker = new Error('distinctive-agent-switch-failure-token-B29c');
+    mockBrain.agents.findByName.mockRejectedValueOnce(marker);
+    const loggerErrorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+
+    const payload: SlashCommandPayload = {
+      command: 'agent',
+      args: 'some-other-agent',
+      conversationId,
+    };
+    const result = await service.execute(payload, userScope);
+
+    expect(result.success).toBe(false);
+    expect(result.command).toBe('agent');
+    expect(result.message).toBe('Failed to switch agent due to an internal error.');
+    expect(result.message).not.toContain('distinctive-agent-switch-failure-token-B29c');
+
+    expect(loggerErrorSpy).toHaveBeenCalled();
+    const loggedRawError = loggerErrorSpy.mock.calls.some((call) => call.includes(marker));
+    expect(loggedRawError).toBe(true);
+
+    loggerErrorSpy.mockRestore();
+  });
+
+  it('/mcp reconnect sanitizes MCP client failures, logging the raw exception but never returning it to the client', async () => {
+    const marker = new Error('distinctive-mcp-reconnect-failure-token-C33e');
+    const mockMcpClient = {
+      reconnectServer: vi.fn().mockRejectedValue(marker),
+      getServerStatuses: vi.fn(() => []),
+      getToolDefinitions: vi.fn(() => []),
+    };
+    const mcpService = buildService(mockRedis, mockMcpClient);
+    const loggerErrorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+
+    const payload: SlashCommandPayload = {
+      command: 'mcp',
+      args: 'reconnect my-server',
+      conversationId,
+    };
+    const result = await mcpService.execute(payload, userScope);
+
+    expect(result.success).toBe(false);
+    expect(result.command).toBe('mcp');
+    expect(result.message).toBe(
+      'Failed to reconnect MCP server "my-server" due to an internal error.',
+    );
+    expect(result.message).not.toContain('distinctive-mcp-reconnect-failure-token-C33e');
+
+    expect(loggerErrorSpy).toHaveBeenCalled();
+    const loggedRawError = loggerErrorSpy.mock.calls.some((call) => call.includes(marker));
+    expect(loggedRawError).toBe(true);
+
+    loggerErrorSpy.mockRestore();
+  });
 });

apps/gateway/src/commands/command-executor.service.ts

@@ -159,8 +159,13 @@ export class CommandExecutorService {
           };
       }
     } catch (err) {
-      this.logger.error(`Command /${command} failed: ${err}`);
-      return { command, conversationId, success: false, message: String(err) };
+      this.logger.error(`Command /${command} failed`, err);
+      return {
+        command,
+        conversationId,
+        success: false,
+        message: 'Command failed due to an internal error.',
+      };
     }
   }
 
@@ -336,11 +341,11 @@ export class CommandExecutorService {
           data: { agentId: newAgent.id, agentName: newAgent.name },
         };
       } catch (err) {
-        this.logger.error(`Failed to create agent: ${err}`);
+        this.logger.error(`Failed to create agent "${namePart}" for user ${userId}`, err);
         return {
           command: 'agent',
           success: false,
-          message: `Failed to create agent: ${String(err)}`,
+          message: 'Failed to create agent due to an internal error.',
           conversationId,
         };
       }
@@ -391,11 +396,11 @@ export class CommandExecutorService {
         data: { agentId: agentConfig.id, agentName: agentConfig.name, model: agentConfig.model },
       };
     } catch (err) {
-      this.logger.error(`Failed to switch agent "${agentName}": ${err}`);
+      this.logger.error(`Failed to switch agent "${agentName}"`, err);
       return {
         command: 'agent',
         success: false,
-        message: `Failed to switch agent: ${String(err)}`,
+        message: 'Failed to switch agent due to an internal error.',
         conversationId,
       };
     }
@@ -608,11 +613,12 @@ export class CommandExecutorService {
             message: `MCP server "${serverName}" reconnected successfully.`,
           };
         } catch (err) {
+          this.logger.error(`Failed to reconnect MCP server "${serverName}"`, err);
           return {
             command: 'mcp',
             conversationId,
             success: false,
-            message: `Failed to reconnect MCP server "${serverName}": ${err instanceof Error ? err.message : String(err)}`,
+            message: `Failed to reconnect MCP server "${serverName}" due to an internal error.`,
           };
         }
       }

apps/gateway/src/mcp-client/mcp-client.service.spec.ts

@@ -0,0 +1,44 @@
+import { Logger } from '@nestjs/common';
+import { Client } from '@modelcontextprotocol/sdk/client/index.js';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { McpClientService } from './mcp-client.service.js';
+
+const MCP_LEAK_MARKER = 'MCP_LEAK_MARKER /srv/secret';
+
+describe('McpClientService — failed connect error sanitization', () => {
+  const originalMcpServers = process.env['MCP_SERVERS'];
+
+  beforeEach(() => {
+    process.env['MCP_SERVERS'] = JSON.stringify([
+      { name: 'leaky-server', url: 'http://localhost:9999/mcp' },
+    ]);
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+    if (originalMcpServers === undefined) {
+      delete process.env['MCP_SERVERS'];
+    } else {
+      process.env['MCP_SERVERS'] = originalMcpServers;
+    }
+  });
+
+  it('stores a generic serverEntry.error while logging the raw exception server-side', async () => {
+    vi.spyOn(Client.prototype, 'connect').mockRejectedValue(new Error(MCP_LEAK_MARKER));
+    const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+
+    const service = new McpClientService();
+    await service.onModuleInit();
+
+    const statuses = service.getServerStatuses();
+    expect(statuses).toHaveLength(1);
+    expect(statuses[0]?.connected).toBe(false);
+    expect(statuses[0]?.error).toBe('Connection failed (see server logs).');
+    expect(statuses[0]?.error).not.toContain(MCP_LEAK_MARKER);
+
+    const loggedRawMarker = errorSpy.mock.calls.some((call) =>
+      call.some((arg) => typeof arg === 'string' && arg.includes(MCP_LEAK_MARKER)),
+    );
+    expect(loggedRawMarker).toBe(true);
+  });
+});

apps/gateway/src/mcp-client/mcp-client.service.ts

@@ -189,7 +189,7 @@ export class McpClientService implements OnModuleInit, OnModuleDestroy {
       );
     } catch (err) {
       const message = err instanceof Error ? err.message : String(err);
-      serverEntry.error = message;
+      serverEntry.error = 'Connection failed (see server logs).';
       serverEntry.connected = false;
       this.logger.error(`Failed to connect to MCP server "${config.name}": ${message}`);
     }

apps/gateway/src/reload/reload.service.spec.ts

@@ -1,5 +1,8 @@
+import { Logger } from '@nestjs/common';
 import { describe, expect, it, vi } from 'vitest';
+import type { SlashCommandPayload, SystemReloadPayload } from '@mosaicstack/types';
 import { ReloadService } from './reload.service.js';
+import { CommandExecutorService } from '../commands/command-executor.service.js';
 
 function createMockCommandRegistry() {
   return {
@@ -104,3 +107,79 @@ describe('ReloadService', () => {
     expect(() => service.registerPlugin('my-plugin', {})).not.toThrow();
   });
 });
+
+describe('ReloadService — /reload command sanitizes plugin errors', () => {
+  it('generic per-plugin errors reach the chat surface while raw markers stay server-side only', async () => {
+    const registry = {
+      getManifest: vi.fn().mockReturnValue({
+        version: 1,
+        commands: [
+          { name: 'reload', aliases: [], scope: 'core', execution: 'socket', available: true },
+        ],
+        skills: [],
+      }),
+    };
+    const reloadService = new ReloadService(registry as never);
+
+    const RELOAD_LOAD_LEAK_MARKER = 'RELOAD_LOAD_LEAK_MARKER /srv/load-secret';
+    const RELOAD_UNLOAD_LEAK_MARKER = 'RELOAD_UNLOAD_LEAK_MARKER /srv/unload-secret';
+
+    reloadService.registerPlugin('unload-fails', {
+      pluginName: 'unload-fails',
+      onLoad: vi.fn().mockResolvedValue(undefined),
+      onUnload: vi.fn().mockRejectedValue(new Error(RELOAD_UNLOAD_LEAK_MARKER)),
+    });
+    reloadService.registerPlugin('load-fails', {
+      pluginName: 'load-fails',
+      onLoad: vi.fn().mockRejectedValue(new Error(RELOAD_LOAD_LEAK_MARKER)),
+      onUnload: vi.fn().mockResolvedValue(undefined),
+    });
+
+    const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined);
+    const broadcastReload = vi.fn();
+    const mockChatGateway = { broadcastReload };
+    const mockAgentService = { getSession: vi.fn(), applyAgentConfig: vi.fn() };
+    const mockSystemOverride = { set: vi.fn(), get: vi.fn(), clear: vi.fn() };
+    const mockSessionGC = { sweepOrphans: vi.fn() };
+    const mockBrain = { agents: { findByName: vi.fn(), findById: vi.fn(), create: vi.fn() } };
+
+    const executor = new CommandExecutorService(
+      registry as never,
+      mockAgentService as never,
+      mockSystemOverride as never,
+      mockSessionGC as never,
+      null,
+      mockBrain as never,
+      reloadService,
+      mockChatGateway as never,
+      null,
+    );
+
+    const payload: SlashCommandPayload = { command: 'reload', conversationId: 'conv-1' };
+    const result = await executor.execute(payload, { userId: 'user-1', tenantId: 'user-1' });
+
+    expect(result.success).toBe(true);
+    expect(result.message).toContain('unload-fails: unload failed (internal error)');
+    expect(result.message).toContain('load-fails: load failed (internal error)');
+    expect(result.message).not.toContain(RELOAD_UNLOAD_LEAK_MARKER);
+    expect(result.message).not.toContain(RELOAD_LOAD_LEAK_MARKER);
+
+    expect(broadcastReload).toHaveBeenCalledOnce();
+    const broadcastPayload = broadcastReload.mock.calls[0]?.[0] as SystemReloadPayload;
+    expect(broadcastPayload.message).toContain('unload-fails: unload failed (internal error)');
+    expect(broadcastPayload.message).toContain('load-fails: load failed (internal error)');
+    expect(broadcastPayload.message).not.toContain(RELOAD_UNLOAD_LEAK_MARKER);
+    expect(broadcastPayload.message).not.toContain(RELOAD_LOAD_LEAK_MARKER);
+
+    const loggedUnloadMarker = errorSpy.mock.calls.some((call) =>
+      call.some((arg) => typeof arg === 'string' && arg.includes(RELOAD_UNLOAD_LEAK_MARKER)),
+    );
+    const loggedLoadMarker = errorSpy.mock.calls.some((call) =>
+      call.some((arg) => typeof arg === 'string' && arg.includes(RELOAD_LOAD_LEAK_MARKER)),
+    );
+    expect(loggedUnloadMarker).toBe(true);
+    expect(loggedLoadMarker).toBe(true);
+
+    errorSpy.mockRestore();
+  });
+});

apps/gateway/src/reload/reload.service.ts

@@ -58,7 +58,8 @@ export class ReloadService implements OnApplicationBootstrap, OnApplicationShutd
           await plugin.onUnload();
           reloaded.push(name);
         } catch (err) {
-          errors.push(`${name}: unload failed — ${err}`);
+          this.logger.error(`Plugin "${name}" failed during onUnload: ${err}`);
+          errors.push(`${name}: unload failed (internal error)`);
         }
       }
     }
@@ -69,7 +70,8 @@ export class ReloadService implements OnApplicationBootstrap, OnApplicationShutd
         try {
           await plugin.onLoad();
         } catch (err) {
-          errors.push(`${name}: load failed — ${err}`);
+          this.logger.error(`Plugin "${name}" failed during onLoad: ${err}`);
+          errors.push(`${name}: load failed (internal error)`);
         }
       }
     }