From 48bb19310dd3b7debc2d527b8e64985d6084c145 Mon Sep 17 00:00:00 2001 From: "shaggy (mosaic-dev box)" Date: Mon, 10 Aug 2026 01:58:27 -0500 Subject: [PATCH] fix(web): close P3 chat re-review findings --- apps/web/src/lib/socket.spec.ts | 41 ++- apps/web/src/spa/chat/commands-panel.spec.tsx | 60 +++ apps/web/src/spa/chat/commands-panel.tsx | 26 +- apps/web/src/spa/chat/runtime-guards.ts | 8 + apps/web/src/spa/chat/session-panel.tsx | 9 +- .../src/spa/chat/use-chat-connection.spec.tsx | 343 +++++++++++++++++- apps/web/src/spa/chat/use-chat-connection.ts | 267 ++++++++++++-- .../spa/pages/chat-error-boundary.spec.tsx | 37 +- apps/web/src/spa/pages/chat.spec.tsx | 45 ++- pnpm-lock.yaml | 3 + 10 files changed, 774 insertions(+), 65 deletions(-) diff --git a/apps/web/src/lib/socket.spec.ts b/apps/web/src/lib/socket.spec.ts index 9a70dda3..6d572df4 100644 --- a/apps/web/src/lib/socket.spec.ts +++ b/apps/web/src/lib/socket.spec.ts @@ -10,25 +10,45 @@ vi.mock('socket.io-client', () => ({ import { destroySocket, getSocket } from './socket'; -function createMockSocket(): { +interface MockChatSocket { on: ReturnType; offAny: ReturnType; disconnect: ReturnType; -} { - const mockSocket = { - on: vi.fn(() => mockSocket), + /** 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', () => { beforeEach(() => { ioMock.mockReset(); // A fresh object per io() call so identity assertions (same singleton vs. // a genuinely new instance) are meaningful. - ioMock.mockImplementation(() => createMockSocket()); + ioMock.mockImplementation(() => { + currentMock = createMockSocket(); + return currentMock; + }); }); afterEach(() => { @@ -52,9 +72,14 @@ describe('chat socket', () => { 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, so - // simply calling getSocket() again after a "disconnect" must still - // return the same instance. + // 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); diff --git a/apps/web/src/spa/chat/commands-panel.spec.tsx b/apps/web/src/spa/chat/commands-panel.spec.tsx index 6d0c7a85..ade87a98 100644 --- a/apps/web/src/spa/chat/commands-panel.spec.tsx +++ b/apps/web/src/spa/chat/commands-panel.spec.tsx @@ -174,6 +174,66 @@ describe('CommandsPanel', () => { ).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('does not throw when the manifest fields are malformed (non-array commands/skills)', async () => { const manifest = { commands: 'not-an-array', diff --git a/apps/web/src/spa/chat/commands-panel.tsx b/apps/web/src/spa/chat/commands-panel.tsx index 79b98af6..b0d50eca 100644 --- a/apps/web/src/spa/chat/commands-panel.tsx +++ b/apps/web/src/spa/chat/commands-panel.tsx @@ -1,14 +1,16 @@ import { useState, type ReactElement } from 'react'; import type { PendingApproval } from './use-chat-connection'; -import { asString } from './runtime-guards'; +import { asNonEmptyString, asString } from './runtime-guards'; import type { CommandManifest, SlashCommandApprovalResultPayload, SlashCommandResultPayload, } from '@/lib/chat-contract'; -/** Stable client copy shown for a failed command — never the raw server - * detail, which could leak internal error text to the user. */ +/** 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.'; interface CommandsPanelProps { @@ -104,11 +106,17 @@ export function CommandsPanel({ {approval ? (
- {/* 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. */} - {approval.success ? 'Approved.' : 'Denied.'} + {/* 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 @@ -135,7 +143,7 @@ export function CommandsPanel({ ? typeof result.message === 'string' && result.message ? ` — ${result.message}` : '' - : ` — ${COMMAND_FAILURE_COPY}`} + : ` — ${asNonEmptyString(result.message, COMMAND_FAILURE_COPY)}`} ))} diff --git a/apps/web/src/spa/chat/runtime-guards.ts b/apps/web/src/spa/chat/runtime-guards.ts index b879f58d..2ea627bd 100644 --- a/apps/web/src/spa/chat/runtime-guards.ts +++ b/apps/web/src/spa/chat/runtime-guards.ts @@ -10,6 +10,14 @@ export function asString(value: unknown, fallback = ''): string { return typeof value === 'string' ? value : fallback; } +/** Like `asString`, but an empty string also falls back — used for guarded + * contract-provided reason strings (e.g. a denial or failure message) where + * an empty string is not a meaningful value to display in place of the + * stable fallback copy. */ +export function asNonEmptyString(value: unknown, fallback: string): string { + return typeof value === 'string' && value.length > 0 ? value : fallback; +} + export function asFiniteNumber(value: unknown, fallback = 0): number { return typeof value === 'number' && Number.isFinite(value) ? value : fallback; } diff --git a/apps/web/src/spa/chat/session-panel.tsx b/apps/web/src/spa/chat/session-panel.tsx index b89422ba..74e38e84 100644 --- a/apps/web/src/spa/chat/session-panel.tsx +++ b/apps/web/src/spa/chat/session-panel.tsx @@ -1,5 +1,6 @@ import type { ReactElement } from 'react'; import type { SessionInfoPayload } from '@/lib/chat-contract'; +import { MAX_MANIFEST_ITEMS } from './limits'; import { asString, asStringArray } from './runtime-guards'; interface SessionPanelProps { @@ -15,7 +16,13 @@ export function SessionPanel({ }: SessionPanelProps): ReactElement | null { if (!sessionInfo) return null; - const availableThinkingLevels = asStringArray(sessionInfo.availableThinkingLevels); + // The reducer already caps this before storing it, but the render site + // defends independently — a hostile payload must never be able to force + // this ) can never be forced to lay out an + // unbounded number of options. + availableThinkingLevels: asStringArray(payload.availableThinkingLevels).slice( + 0, + MAX_MANIFEST_ITEMS, + ), + }, + }; } case 'server/commands:manifest': { @@ -512,12 +671,24 @@ function reduce(state: ChatConnectionState, action: Action): ChatConnectionState case 'server/system:reload': { const { payload } = action; + // Computed once and reused for both `systemReload` and `manifest` — + // spreading the raw `payload` into `systemReload` first and only + // capping the copy handed to `manifest` left the raw, uncapped + // commands/skills sitting in `state.systemReload`. The sanitized + // fields are placed after the spread below so they always win. + const commands = capList(payload.commands, MAX_MANIFEST_ITEMS); + const skills = capList(payload.skills, MAX_MANIFEST_ITEMS); return { ...state, - systemReload: { ...payload, message: asString(payload.message, 'Commands reloaded.') }, + systemReload: { + ...payload, + commands, + skills, + message: asString(payload.message, 'Commands reloaded.'), + }, manifest: { - commands: capList(payload.commands, MAX_MANIFEST_ITEMS), - skills: capList(payload.skills, MAX_MANIFEST_ITEMS), + commands, + skills, version: state.manifest?.version ?? 0, }, }; @@ -534,16 +705,31 @@ function reduce(state: ChatConnectionState, action: Action): ChatConnectionState sending: false, streaming: false, approvalRequestPending: false, + armedTurnToken: null, error: CONVERSATION_START_FAILURE, }; } return state; } + const next = resolved.state; + if (isRecognizedStaleTerminal(next)) { + // Same true-no-op guard as `server/agent:end` above: a + // same-conversation error recognized as stale must not display/store + // its message, touch streaming, or clear approvalRequestPending — + // doing so would re-arm the approve UI for a request that belongs to + // the still in-flight later turn while the first remains outstanding. + return next; + } + // Same fail-closed turn-token guard as `server/agent:end` above: a + // same-conversation error left over from an earlier, already-finished + // turn must not release a later turn's still-in-flight send lock. + const canRelease = next.armedTurnToken === next.turnToken; return { - ...resolved.state, + ...next, error: asString(payload.error, 'An error occurred.'), streaming: false, - sending: false, + sending: canRelease ? false : next.sending, + armedTurnToken: canRelease ? null : next.armedTurnToken, // A turn-scoped error also invalidates any approval request awaiting // a response — the gateway that just errored is unlikely to still // answer it, and the synchronous approveLockRef mirrors this field. @@ -566,6 +752,12 @@ function reduce(state: ChatConnectionState, action: Action): ChatConnectionState messageSeq: state.messageSeq + 1, error: null, sending: true, + // Sending a new turn mints a fresh token and immediately clears + // terminal eligibility — only THIS turn's own accepted ack/start may + // arm it, so a same-conversation terminal left over from the turn + // that just finished can never be mistaken for this one's. + turnToken: state.turnToken + 1, + armedTurnToken: null, pendingSend: state.conversationId === null ? true : state.pendingSend, }; } @@ -579,6 +771,7 @@ function reduce(state: ChatConnectionState, action: Action): ChatConnectionState sending: false, pendingSend: false, approvalRequestPending: false, + armedTurnToken: null, }; } @@ -766,12 +959,24 @@ export function useChatConnection(): ChatConnectionValue { if (approval?.success !== true) return; if (typeof approval.approvalId !== 'string' || approval.approvalId.length === 0) return; if (!pendingApproval || pendingApproval.command !== approval.command) return; - if (executedApprovalIds.current.has(approval.approvalId)) return; + if (executedApprovalIds.current.has(approval.approvalId)) { + dispatch({ type: 'local/consume-approval' }); + return; + } if (executedApprovalIds.current.size >= MAX_EXECUTED_APPROVAL_IDS) { - // Bounded dedup set: evict the oldest entry (Sets iterate in - // insertion order) so this can never grow without limit. - const oldest = executedApprovalIds.current.values().next().value; - if (oldest !== undefined) executedApprovalIds.current.delete(oldest); + // Security tradeoff, chosen deliberately: this dedup set never + // evicts. Evicting the oldest entry to make room (the old behavior) + // would let a replay of that forgotten ID execute again once it + // scrolled out of the set — a false negative that lets a privileged + // command run twice. Once the set is full, every *unseen* approval + // is denied instead. The mounted hook remains fail-closed until + // unmounted (lifecycle reset), not recoverable by re-approving — a + // false positive/availability cost but a genuine replay of any ID + // ever seen by this hook can never execute a second time. Consuming + // (rather than silently no-op'ing) releases the UI lock so the + // denial is visible/recoverable by remounting. + dispatch({ type: 'local/consume-approval' }); + return; } executedApprovalIds.current.add(approval.approvalId); const socket = getSocket(); diff --git a/apps/web/src/spa/pages/chat-error-boundary.spec.tsx b/apps/web/src/spa/pages/chat-error-boundary.spec.tsx index 0f480967..1b627491 100644 --- a/apps/web/src/spa/pages/chat-error-boundary.spec.tsx +++ b/apps/web/src/spa/pages/chat-error-boundary.spec.tsx @@ -2,7 +2,16 @@ import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { createMemoryRouter, RouterProvider, type RouteObject } from 'react-router-dom'; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; -import { ChatRouteErrorBoundary } from './chat-error-boundary'; + +const { useSessionMock } = vi.hoisted(() => ({ + useSessionMock: vi.fn(), +})); + +vi.mock('@/lib/auth-client', () => ({ + useSession: useSessionMock, +})); + +import { routes } from '@/routes'; beforeAll(() => { Object.defineProperty(globalThis, 'IS_REACT_ACT_ENVIRONMENT', { @@ -19,6 +28,25 @@ function Boom(): never { throw new Error('render blew up'); } +/** Recursively clones the real exported route table, replacing only the + * `/chat` route's `element` with `` — every other route (including + * the real `AuthGuard` nesting and the real `/chat` `errorElement`) is left + * exactly as exported. This is what makes the test fail if a future change + * removes the real route's `errorElement`, unlike a hand-built independent + * route tree that could drift from production undetected. */ +function replaceChatElementWithBoom(nodes: RouteObject[]): RouteObject[] { + return nodes.map((node) => { + const cloned: RouteObject = { ...node }; + if (cloned.path === '/chat') { + cloned.element = ; + } + if (cloned.children) { + cloned.children = replaceChatElementWithBoom(cloned.children); + } + return cloned; + }); +} + let root: Root | null; let container: HTMLElement; @@ -28,13 +56,14 @@ afterEach(async () => { }); document.body.replaceChildren(); root = null; + useSessionMock.mockReset(); }); describe('ChatRouteErrorBoundary', () => { it('renders a recoverable, non-blank fallback when the /chat route element throws during render', async () => { - const routeObjects: RouteObject[] = [ - { path: '/chat', element: , errorElement: }, - ]; + useSessionMock.mockReturnValue({ data: { user: { id: 'user-1' } }, isPending: false }); + + const routeObjects = replaceChatElementWithBoom(routes); const router = createMemoryRouter(routeObjects, { initialEntries: ['/chat'] }); container = document.createElement('div'); diff --git a/apps/web/src/spa/pages/chat.spec.tsx b/apps/web/src/spa/pages/chat.spec.tsx index fec2c15b..3806564b 100644 --- a/apps/web/src/spa/pages/chat.spec.tsx +++ b/apps/web/src/spa/pages/chat.spec.tsx @@ -2,6 +2,7 @@ import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { createFakeChatSocket } from '@/spa/chat/test-support/fake-chat-socket'; +import { MAX_MANIFEST_ITEMS } from '@/spa/chat/limits'; const { getSocketMock, destroySocketMock } = vi.hoisted(() => ({ getSocketMock: vi.fn(), @@ -233,7 +234,7 @@ describe('ChatPage', () => { }); }); - it('shows visible alert surfaces for a server error and a stable failure copy for a failed command result', async () => { + it('shows visible alert surfaces for a server error and the structured contract reason for a failed command result', async () => { await act(async () => { fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' }); fake.serverEmit('error', { conversationId: 'c1', error: 'The model is unavailable' }); @@ -241,16 +242,52 @@ describe('ChatPage', () => { conversationId: 'c1', command: 'model', success: false, - message: 'raw internal detail: stack trace at line 42', + message: 'Unknown model', }); }); const alerts = [...container.querySelectorAll('[role="alert"]')]; const alertText = alerts.map((node) => node.textContent).join(' '); expect(alertText).toContain('The model is unavailable'); - // Sanitized client copy, not the raw server-provided detail. + // The structured, contract-provided denial reason is visibly rendered. + expect(alertText).toContain('Unknown model'); + }); + + it('falls back to a stable "Command failed." copy when a failed command result has no usable message', async () => { + await act(async () => { + fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' }); + fake.serverEmitRaw('command:result', { + conversationId: 'c1', + command: 'model', + success: false, + message: { bad: 'object' }, + }); + }); + + const alerts = [...container.querySelectorAll('[role="alert"]')]; + const alertText = alerts.map((node) => node.textContent).join(' '); expect(alertText).toContain('Command failed.'); - expect(alertText).not.toContain('raw internal detail'); + }); + + it('caps availableThinkingLevels before storing and rendering a hostile session payload', async () => { + const hostileLevels = Array.from({ length: MAX_MANIFEST_ITEMS + 50 }, (_, i) => `level-${i}`); + + await act(async () => { + fake.serverEmit('message:ack', { conversationId: 'c1', messageId: 'm1' }); + fake.serverEmit('session:info', { + conversationId: 'c1', + provider: 'anthropic', + modelId: 'claude', + thinkingLevel: 'level-0', + availableThinkingLevels: hostileLevels, + }); + }); + + const select = container.querySelector( + 'select[aria-label="Thinking level"]', + ) as HTMLSelectElement; + expect(select).toBeTruthy(); + expect(select.options.length).toBeLessThanOrEqual(MAX_MANIFEST_ITEMS); }); it('renders a safe fallback when session:info arrives with a malformed (non-array) availableThinkingLevels, without throwing', async () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9e9da218..fcfe530e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -253,6 +253,9 @@ importers: '@mosaicstack/design-tokens': specifier: workspace:^ version: link:../../packages/design-tokens + '@mosaicstack/types': + specifier: workspace:^ + version: link:../../packages/types better-auth: specifier: ^1.5.5 version: 1.5.5(better-sqlite3@12.8.0)(drizzle-kit@0.31.9)(drizzle-orm@0.45.1(@electric-sql/pglite@0.2.17)(@opentelemetry/api@1.9.0)(@types/better-sqlite3@7.6.13)(@types/pg@8.15.6)(better-sqlite3@12.8.0)(kysely@0.28.11)(postgres@3.4.8))(mongodb@7.1.0(socks@2.8.7))(next@16.1.6(@opentelemetry/api@1.9.0)(@playwright/test@1.58.2)(react-dom@19.2.4(react@19.2.4))(react@19.2.4))(react-dom@19.2.4(react@19.2.4))(react@19.2.4)(vitest@3.2.7(@types/debug@4.1.13)(@types/node@22.19.15)(jsdom@29.0.0(@noble/hashes@2.0.1))(lightningcss@1.33.0))