diff --git a/apps/gateway/src/commands/command-executor-p8012.spec.ts b/apps/gateway/src/commands/command-executor-p8012.spec.ts index 59f0ed6a..c8165c39 100644 --- a/apps/gateway/src/commands/command-executor-p8012.spec.ts +++ b/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; + getServerStatuses: ReturnType; + getToolDefinitions: ReturnType; + } | 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 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(); }); diff --git a/apps/gateway/src/commands/command-executor.service.ts b/apps/gateway/src/commands/command-executor.service.ts index 4388ecaa..29066ab9 100644 --- a/apps/gateway/src/commands/command-executor.service.ts +++ b/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.`, }; } } diff --git a/apps/web/src/spa/chat/use-chat-connection.spec.tsx b/apps/web/src/spa/chat/use-chat-connection.spec.tsx index d340c40a..545516e2 100644 --- a/apps/web/src/spa/chat/use-chat-connection.spec.tsx +++ b/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' }); diff --git a/apps/web/src/spa/chat/use-chat-connection.ts b/apps/web/src/spa/chat/use-chat-connection.ts index 9f85ad99..b2a81dd2 100644 --- a/apps/web/src/spa/chat/use-chat-connection.ts +++ b/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.'),