fix(web,gateway): close P3 re-review#4 findings — sanitize all executor catches; lock pre-start turn boundary (wire turnId deferred)

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

@@ -13,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: [],
   })),
@@ -73,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,
@@ -83,7 +91,7 @@ function buildService(redis: typeof mockRedis | null = mockRedis): CommandExecut
     mockBrain as never,
     null,
     mockChatGateway as never,
-    null,
+    mcpClient as never,
   );
 }
 
@@ -266,8 +274,9 @@ describe('CommandExecutorService — P8-012 commands', () => {
   // 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(new Error(distinctiveRawFailure)),
+      set: vi.fn().mockRejectedValue(rawError),
       get: vi.fn(),
       del: vi.fn(),
     };
@@ -287,10 +296,94 @@ describe('CommandExecutorService — P8-012 commands', () => {
     expect(result.message).not.toContain(distinctiveRawFailure);
     expect(result.message).not.toContain('ECONNREFUSED');
 
-    // The real exception is still logged server-side.
+    // 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 loggedText = loggerErrorSpy.mock.calls.map((call) => String(call[0])).join(' ');
-    expect(loggedText).toContain(distinctiveRawFailure);
+    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,7 +159,7 @@ export class CommandExecutorService {
           };
       }
     } catch (err) {
-      this.logger.error(`Command /${command} failed: ${err}`);
+      this.logger.error(`Command /${command} failed`, err);
       return {
         command,
         conversationId,
@@ -341,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,
         };
       }
@@ -396,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,
       };
     }
@@ -613,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/web/src/spa/chat/use-chat-connection.spec.tsx

@@ -1587,7 +1587,14 @@ describe('useChatConnection', () => {
     expect(fake.emitted.filter((e) => e.event === 'command:approve')).toHaveLength(2);
   });
 
-  it("does not settle turn B when a duplicate agent:end from already-settled turn A is redelivered after B's own ack but before B's own start", async () => {
+  it("P3-5c: locks the achievable pre-start boundary — turn A settled, turn B sent and acked but before B's own start, a stale/duplicate agent:end from A does not release B (a third send stays blocked), and only B's own start/end sequence legitimately releases it", async () => {
+    // This is the provable boundary: in-order Socket.IO delivery guarantees
+    // a same-conversation agent:end arriving before this turn's own
+    // agent:start can only be a stale straggler. Once a turn is 'active',
+    // AgentEndPayload/ErrorPayload carry no turn identity to further
+    // distinguish a genuine end from a duplicate — that residual is not,
+    // and cannot be, asserted here; full correlation needs a wire turnId
+    // (deferred to P5). See the reducer comments in use-chat-connection.ts.
     // Turn A completes normally on c1.
     await act(async () => {
       fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' });

apps/web/src/spa/chat/use-chat-connection.ts

@@ -586,6 +586,13 @@ function reduce(state: ChatConnectionState, action: Action): ChatConnectionState
         // still in-flight (or already-finalized) current turn.
         return next;
       }
+      // Reaching here means turnPhase is 'active' — but AgentEndPayload
+      // carries only a conversationId, no turn/message identity. In-order
+      // delivery proves the boundary enforced above (a same-conversation end
+      // arriving before this turn's own agent:start is provably stale); once
+      // 'active', a genuine end for this turn and a same-conversation
+      // stale/duplicate end are indistinguishable here. This residual is
+      // accepted — full correlation requires a wire turnId, deferred to P5.
       const hasContent = next.text.length > 0 || next.thinking.length > 0;
       const messages = hasContent
         ? capPush(
@@ -751,6 +758,13 @@ function reduce(state: ChatConnectionState, action: Action): ChatConnectionState
       // nothing further remains in flight for it — so a scoped error seen
       // while turnPhase is 'pending' or 'active' can only belong to THIS
       // turn, and must settle and release the lock.
+      //
+      // Same wire limitation as agent:end above: ErrorPayload also carries
+      // only a conversationId, no turn identity. A pre-ack ('pending') error
+      // is accepted as current by design, per the reasoning above. Once
+      // 'active', a genuine error for this turn and a same-conversation
+      // stale/duplicate error are equally indistinguishable — accepted
+      // residual; full correlation needs a wire turnId, deferred to P5.
       return {
         ...next,
         error: asString(payload.error, 'An error occurred.'),