diff --git a/apps/gateway/src/commands/command-executor-p8012.spec.ts b/apps/gateway/src/commands/command-executor-p8012.spec.ts index b22ed05a..c8165c39 100644 --- a/apps/gateway/src/commands/command-executor-p8012.spec.ts +++ b/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; + getServerStatuses: ReturnType; + getToolDefinitions: ReturnType; + } | 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 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 ca2d3cfd..29066ab9 100644 --- a/apps/gateway/src/commands/command-executor.service.ts +++ b/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.`, }; } } diff --git a/apps/gateway/src/mcp-client/mcp-client.service.spec.ts b/apps/gateway/src/mcp-client/mcp-client.service.spec.ts new file mode 100644 index 00000000..531299c9 --- /dev/null +++ b/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); + }); +}); diff --git a/apps/gateway/src/mcp-client/mcp-client.service.ts b/apps/gateway/src/mcp-client/mcp-client.service.ts index 28910e0d..6c304f70 100644 --- a/apps/gateway/src/mcp-client/mcp-client.service.ts +++ b/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}`); } diff --git a/apps/gateway/src/reload/reload.service.spec.ts b/apps/gateway/src/reload/reload.service.spec.ts index 25696941..f939872a 100644 --- a/apps/gateway/src/reload/reload.service.spec.ts +++ b/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(); + }); +}); diff --git a/apps/gateway/src/reload/reload.service.ts b/apps/gateway/src/reload/reload.service.ts index 1d84e27b..b6b1f269 100644 --- a/apps/gateway/src/reload/reload.service.ts +++ b/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)`); } } } diff --git a/apps/web/package.json b/apps/web/package.json index e7442a4e..0e1efee9 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -15,6 +15,7 @@ }, "dependencies": { "@mosaicstack/design-tokens": "workspace:^", + "@mosaicstack/types": "workspace:^", "better-auth": "^1.5.5", "clsx": "^2.1.0", "next": "^16.0.0", diff --git a/apps/web/src/lib/chat-contract.ts b/apps/web/src/lib/chat-contract.ts new file mode 100644 index 00000000..f8da2fc3 --- /dev/null +++ b/apps/web/src/lib/chat-contract.ts @@ -0,0 +1,61 @@ +// Centralizes the type-only import of the shared `/chat` Socket.IO contract from +// the public `@mosaicstack/types` package. `import type` is erased at compile +// time, so this introduces no runtime dependency — it only reuses the exact +// payload shapes instead of redeclaring them. +import type { Socket } from 'socket.io-client'; +import type { + AbortPayload, + AgentEndPayload, + AgentStartPayload, + AgentTextPayload, + AgentThinkingPayload, + ChatMessagePayload, + ClientToServerEvents, + CommandDef, + CommandManifest, + CommandManifestPayload, + ErrorPayload, + MessageAckPayload, + RoutingDecisionInfo, + ServerToClientEvents, + SessionInfoPayload, + SessionUsagePayload, + SetThinkingPayload, + SkillCommandDef, + SlashCommandApprovalResultPayload, + SlashCommandPayload, + SlashCommandResultPayload, + SystemReloadPayload, + ToolEndPayload, + ToolStartPayload, +} from '@mosaicstack/types'; + +export type { + AbortPayload, + AgentEndPayload, + AgentStartPayload, + AgentTextPayload, + AgentThinkingPayload, + ChatMessagePayload, + ClientToServerEvents, + CommandDef, + CommandManifest, + CommandManifestPayload, + ErrorPayload, + MessageAckPayload, + RoutingDecisionInfo, + ServerToClientEvents, + SessionInfoPayload, + SessionUsagePayload, + SetThinkingPayload, + SkillCommandDef, + SlashCommandApprovalResultPayload, + SlashCommandPayload, + SlashCommandResultPayload, + SystemReloadPayload, + ToolEndPayload, + ToolStartPayload, +}; + +/** The `/chat` namespace socket, narrowed to the exact typed event contract. */ +export type ChatSocket = Socket; diff --git a/apps/web/src/lib/socket.spec.ts b/apps/web/src/lib/socket.spec.ts index 815b2472..6d572df4 100644 --- a/apps/web/src/lib/socket.spec.ts +++ b/apps/web/src/lib/socket.spec.ts @@ -10,30 +10,52 @@ vi.mock('socket.io-client', () => ({ import { destroySocket, getSocket } from './socket'; +interface MockChatSocket { + on: ReturnType; + offAny: ReturnType; + disconnect: ReturnType; + /** Test-only helper: fires every handler registered for `event` via + * `.on`, mirroring how a real socket.io-client instance invokes its own + * listeners (e.g. calling the registered `disconnect` handler(s) on a + * real transient disconnect). */ + trigger(event: string): void; +} + +function createMockSocket(): MockChatSocket { + const handlers = new Map void>>(); + const mockSocket: MockChatSocket = { + on: vi.fn((event: string, handler: () => void) => { + if (!handlers.has(event)) handlers.set(event, new Set()); + handlers.get(event)?.add(handler); + return mockSocket; + }), + offAny: vi.fn(() => mockSocket), + disconnect: vi.fn(() => mockSocket), + trigger(event: string): void { + for (const handler of handlers.get(event) ?? []) handler(); + }, + }; + return mockSocket; +} + +let currentMock!: MockChatSocket; + describe('chat socket', () => { - let disconnectHandler: (() => void) | undefined; - beforeEach(() => { - disconnectHandler = undefined; ioMock.mockReset(); - - const mockSocket = { - on: vi.fn((event: string, handler: () => void) => { - if (event === 'disconnect') disconnectHandler = handler; - return mockSocket; - }), - offAny: vi.fn(() => mockSocket), - disconnect: vi.fn(() => mockSocket), - }; - - ioMock.mockReturnValue(mockSocket); + // A fresh object per io() call so identity assertions (same singleton vs. + // a genuinely new instance) are meaningful. + ioMock.mockImplementation(() => { + currentMock = createMockSocket(); + return currentMock; + }); }); afterEach(() => { destroySocket(); }); - it('creates one same-origin /chat namespace socket until it disconnects', () => { + it('creates one same-origin /chat namespace socket', () => { const first = getSocket(); const second = getSocket(); @@ -44,9 +66,33 @@ describe('chat socket', () => { autoConnect: false, transports: ['websocket', 'polling'], }); + }); - disconnectHandler?.(); - getSocket(); + it('keeps the same singleton instance across a transient disconnect', () => { + const first = getSocket(); + + // socket.ts must not react to a real socket's `disconnect` event by + // nulling the singleton — it registers no such handler at all now. + // Actually fire every handler registered via `.on('disconnect', ...)` + // (mirroring a real socket.io-client reconnect) instead of merely + // calling getSocket() again: this is what makes the test fail if + // production reintroduces `socket.on('disconnect', () => { socket = + // null; })`, since that handler would run here and null the singleton + // before the next getSocket() call. + currentMock.trigger('disconnect'); + const second = getSocket(); + + expect(second).toBe(first); + expect(ioMock).toHaveBeenCalledOnce(); + }); + + it('only creates a new singleton after an explicit destroySocket()', () => { + const first = getSocket(); + + destroySocket(); + const second = getSocket(); + + expect(second).not.toBe(first); expect(ioMock).toHaveBeenCalledTimes(2); }); }); diff --git a/apps/web/src/lib/socket.ts b/apps/web/src/lib/socket.ts index 66cf64f4..62722076 100644 --- a/apps/web/src/lib/socket.ts +++ b/apps/web/src/lib/socket.ts @@ -1,21 +1,27 @@ -import { io, type Socket } from 'socket.io-client'; +import { io } from 'socket.io-client'; +import type { ChatSocket } from './chat-contract'; -let socket: Socket | null = null; +let socket: ChatSocket | null = null; -export function getSocket(): Socket { +export function getSocket(): ChatSocket { if (!socket) { + // socket.io-client 4.8.3's `io()` factory declaration always returns the + // default unparameterized Socket (it accepts no + // generics), so this one cast is the unavoidable boundary between that and the + // typed `/chat` contract. Every other call site uses the resulting ChatSocket + // with no further assertions. socket = io('/chat', { withCredentials: true, autoConnect: false, transports: ['websocket', 'polling'], - }); + }) as unknown as ChatSocket; - // Reset singleton reference when socket is fully closed so the next - // getSocket() call creates a fresh instance instead of returning a - // closed/dead socket. - socket.on('disconnect', () => { - socket = null; - }); + // A transient `disconnect` (network blip, server restart) must NOT null + // the singleton: socket.io-client auto-reconnects this same instance, + // and its listeners stay registered across that reconnect. Nulling here + // previously orphaned those listeners on the next getSocket() call by + // handing back a brand-new, unconnected instance. Only destroySocket() + // (an explicit, intentional teardown) may reset the singleton. } return socket; } diff --git a/apps/web/src/routes.tsx b/apps/web/src/routes.tsx index 20556bad..dc852e38 100644 --- a/apps/web/src/routes.tsx +++ b/apps/web/src/routes.tsx @@ -3,6 +3,8 @@ import { createBrowserRouter, Navigate, Outlet, type RouteObject } from 'react-r import { LoginPage } from '@/spa/pages/login'; import { RegisterPage } from '@/spa/pages/register'; import { SsoCallbackPage } from '@/spa/pages/sso-callback'; +import { ChatPage } from '@/spa/pages/chat'; +import { ChatRouteErrorBoundary } from '@/spa/pages/chat-error-boundary'; import { AuthGuard, GuestGuard } from '@/spa/guards'; import { Placeholder } from '@/spa/placeholder'; @@ -34,7 +36,7 @@ export const routes: RouteObject[] = [ element: , children: [ { path: '/', element: }, - { path: '/chat', element: }, + { path: '/chat', element: , errorElement: }, { path: '/projects', element: }, { path: '/projects/:id', element: }, { path: '/tasks', element: }, diff --git a/apps/web/src/spa/chat/commands-panel.spec.tsx b/apps/web/src/spa/chat/commands-panel.spec.tsx new file mode 100644 index 00000000..fccc3a08 --- /dev/null +++ b/apps/web/src/spa/chat/commands-panel.spec.tsx @@ -0,0 +1,280 @@ +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { CommandsPanel } from './commands-panel'; + +beforeAll(() => { + Object.defineProperty(globalThis, 'IS_REACT_ACT_ENVIRONMENT', { + configurable: true, + value: true, + }); +}); + +afterAll(() => { + Reflect.deleteProperty(globalThis, 'IS_REACT_ACT_ENVIRONMENT'); +}); + +let root: Root | null; +let container: HTMLElement | null; + +async function render(node: Parameters[0]): Promise { + container = document.createElement('div'); + document.body.append(container); + root = createRoot(container); + await act(async () => { + root?.render(node); + }); +} + +afterEach(async () => { + await act(async () => { + root?.unmount(); + }); + document.body.replaceChildren(); + root = null; + container = null; +}); + +describe('CommandsPanel', () => { + it('shows the frozen local pendingApproval args in the confirmation area, regardless of misleading server message text', async () => { + await render( + , + ); + + // The exact frozen combined action is visible... + expect(container?.textContent).toContain('/deploy'); + expect(container?.textContent).toContain('prod'); + // ...and the misleading server free-text is never shown next to it. + expect(container?.textContent).not.toContain('staging environment'); + }); + + it('does not throw when a manifest commands entry is null', async () => { + const manifest = { + commands: [ + null, + { + name: 'model', + aliases: [], + description: 'Change the active model', + scope: 'core', + execution: 'socket', + available: true, + }, + ], + skills: [null], + version: 1, + } as unknown as Parameters[0]['manifest']; + + await expect( + render( + , + ), + ).resolves.not.toThrow(); + + expect(container?.textContent).toContain('model'); + }); + + it('shows an explicit no-args fallback when the frozen pendingApproval has no args', async () => { + await render( + , + ); + + expect(container?.textContent?.toLowerCase()).toContain('no args'); + }); + + it('renders skills from a skills-only manifest', async () => { + await render( + , + ); + + expect(container?.textContent).toContain('brave-search'); + expect(container?.textContent).toContain('Search the web'); + }); + + it('does not show the Run affordance when approval.success/approvalId are objects, even though command matches pendingApproval', async () => { + const approval = { + conversationId: 'c1', + command: 'deploy', + success: { truthy: 'object' }, + approvalId: { also: 'object' }, + } as unknown as Parameters[0]['approval']; + + await render( + , + ); + + expect( + [...(container?.querySelectorAll('button') ?? [])].some((button) => + button.textContent?.includes('Run approved command'), + ), + ).toBe(false); + }); + + it('shows the guarded server-provided denial reason for a denied approval', async () => { + await render( + , + ); + + expect(container?.textContent).toContain('Not authorized'); + }); + + it('falls back to a stable "Denied." copy when a denial has no usable message', async () => { + await render( + , + ); + + expect(container?.textContent).toContain('Denied.'); + }); + + it('shows the guarded contract-provided reason for a failed command result, falling back to a stable copy only when absent', async () => { + await render( + , + ); + + expect(container?.textContent).toContain('Unknown model'); + expect(container?.textContent).toContain('Command failed.'); + }); + + it('bounds an oversized command result message at the render site as defense-in-depth', async () => { + const hostileMessage = 'y'.repeat(50_000); + await render( + , + ); + + const text = container?.textContent ?? ''; + expect(text.length).toBeLessThan(hostileMessage.length); + }); + + it('does not throw when the manifest fields are malformed (non-array commands/skills)', async () => { + const manifest = { + commands: 'not-an-array', + skills: null, + version: 1, + } as unknown as Parameters[0]['manifest']; + + await expect( + render( + , + ), + ).resolves.not.toThrow(); + }); +}); diff --git a/apps/web/src/spa/chat/commands-panel.tsx b/apps/web/src/spa/chat/commands-panel.tsx new file mode 100644 index 00000000..827f17e1 --- /dev/null +++ b/apps/web/src/spa/chat/commands-panel.tsx @@ -0,0 +1,164 @@ +import { useState, type ReactElement } from 'react'; +import type { PendingApproval } from './use-chat-connection'; +import { MAX_COMMAND_MESSAGE_CHARS } from './limits'; +import { asNonEmptyString, asString } from './runtime-guards'; +import type { + CommandManifest, + SlashCommandApprovalResultPayload, + SlashCommandResultPayload, +} from '@/lib/chat-contract'; + +/** Stable fallback copy shown for a failed command only when the server's + * own guarded, non-empty `message` (e.g. "Unknown model") is absent or + * malformed — the structured contract reason itself is otherwise shown + * directly, never a raw thrown exception, stack trace, or object value. */ +const COMMAND_FAILURE_COPY = 'Command failed.'; + +/** Render-site defense-in-depth: `use-chat-connection.ts` already bounds a + * stored command:result message at ingestion, but this component must never + * assume every caller went through that path — bounding again here means a + * hostile/oversized message can never force an unbounded render. */ +function boundMessage(value: string): string { + return value.length > MAX_COMMAND_MESSAGE_CHARS + ? value.slice(0, MAX_COMMAND_MESSAGE_CHARS) + : value; +} + +interface CommandsPanelProps { + manifest: CommandManifest | null; + results: SlashCommandResultPayload[]; + approval: SlashCommandApprovalResultPayload | null; + pendingApproval: PendingApproval | null; + hasConversation: boolean; + onExecute: (input: { command: string; args?: string }) => void; + onApprove: (input: { command: string; args?: string }) => void; + onRunApproved: () => void; +} + +export function CommandsPanel({ + manifest, + results, + approval, + pendingApproval, + hasConversation, + onExecute, + onApprove, + onRunApproved, +}: CommandsPanelProps): ReactElement { + const [command, setCommand] = useState(''); + const [args, setArgs] = useState(''); + + // Defense-in-depth: the reducer already normalizes success/approvalId + // before storing `approval`, but a matching command string alone must + // never be trusted here either — require the literal boolean `true` and a + // non-empty string approvalId, not merely truthy values. + const canRunApproved = + approval?.success === true && + typeof approval.approvalId === 'string' && + approval.approvalId.length > 0 && + !!pendingApproval && + pendingApproval.command === approval.command; + + // A manifest arrives from the server as untyped JSON at runtime — guard + // both collections before mapping so a malformed manifest cannot throw. + const commands = Array.isArray(manifest?.commands) ? manifest.commands : []; + const skills = Array.isArray(manifest?.skills) ? manifest.skills : []; + + return ( +
+ {commands.length > 0 ? ( +
    + {commands.map((cmd, index) => ( +
  • + /{asString(cmd?.name)} — {asString(cmd?.description)} +
    • + ))} +
+ ) : null} + + {skills.length > 0 ? ( +
    + {skills.map((skill, index) => ( +
  • + /skill:{asString(skill?.name)} — {asString(skill?.description)} +
    • + ))} +
+ ) : null} + +
+ setCommand(event.target.value)} + placeholder="command" + /> + setArgs(event.target.value)} + placeholder="args (optional)" + /> + + +
+ + {approval ? ( +
+ {/* A successful approval shows stable client copy only — never + the server-controlled approval.message or echoed + approval.command as the primary confirmation. The frozen local + pendingApproval below (not this line) is the sole authoritative + statement of what will run. A denial, by contrast, is not an + execution authority and safely surfaces the guarded structured + reason the server gave (e.g. "Not authorized"), falling back to + a stable copy only when absent/malformed. */} + + {approval.success ? 'Approved.' : asNonEmptyString(approval.message, 'Denied.')} + + {canRunApproved && pendingApproval ? ( + <> + {/* Authoritative frozen local command+args — what the click below + will actually emit. The server's `approval` above is display-only + and must never be trusted to represent the executed payload. */} + + Will run: /{pendingApproval.command}{' '} + {pendingApproval.args ? pendingApproval.args : '(no args)'} + + + + ) : null} +
+ ) : null} + + {results.length > 0 ? ( +
    + {results.map((result, index) => ( +
  • + /{asString(result.command)}: {result.success ? 'success' : 'failed'} + {result.success + ? typeof result.message === 'string' && result.message + ? ` — ${boundMessage(result.message)}` + : '' + : ` — ${boundMessage(asNonEmptyString(result.message, COMMAND_FAILURE_COPY))}`} +
    • + ))} +
+ ) : null} +
+ ); +} diff --git a/apps/web/src/spa/chat/composer.tsx b/apps/web/src/spa/chat/composer.tsx new file mode 100644 index 00000000..7bda88ef --- /dev/null +++ b/apps/web/src/spa/chat/composer.tsx @@ -0,0 +1,98 @@ +import { useState, type KeyboardEvent, type ReactElement } from 'react'; + +interface ComposerProps { + onSend: (input: { content: string; provider?: string; modelId?: string }) => void; + onStop: () => void; + streaming: boolean; + /** True from local send time through server turn startup/ack and + * throughout streaming — a superset of `streaming` that also covers the + * pre-ack window where a second send could otherwise slip through. */ + sending: boolean; + hasConversation: boolean; +} + +export function Composer({ + onSend, + onStop, + streaming, + sending, + hasConversation, +}: ComposerProps): ReactElement { + const [content, setContent] = useState(''); + const [provider, setProvider] = useState(''); + const [modelId, setModelId] = useState(''); + const busy = streaming || sending; + + function submit(): void { + if (busy) return; + const trimmed = content.trim(); + if (!trimmed) return; + onSend({ + content: trimmed, + provider: provider.trim() || undefined, + modelId: modelId.trim() || undefined, + }); + setContent(''); + } + + function handleKeyDown(event: KeyboardEvent): void { + if (event.key === 'Enter' && !event.shiftKey) { + event.preventDefault(); + submit(); + } + } + + return ( +
{ + event.preventDefault(); + submit(); + }} + className="flex flex-col gap-2 border-t p-4" + > +
+ setProvider(event.target.value)} + placeholder="Provider (optional)" + className="rounded border px-2 py-1 text-xs" + /> + setModelId(event.target.value)} + placeholder="Model (optional)" + className="rounded border px-2 py-1 text-xs" + /> +
+
+