diff --git a/website/src/pages/ChatPage.tsx b/website/src/pages/ChatPage.tsx index 92e28e1ab6f..5e1a86ac00d 100644 --- a/website/src/pages/ChatPage.tsx +++ b/website/src/pages/ChatPage.tsx @@ -12,6 +12,7 @@ import { settingsPath } from '../components/settingsPath' import { isTouchDevice } from '../utils/isTouchDevice' import { isBrowseCommand } from '../utils/browseCommand' import { isHiddenInvisibleAssistantRow } from '../utils/invisibleText' +import { mergeRenderers, resolveRenderer, type MessageRenderer, type MessageRenderContext } from '../app-sdk/messageRenderers' // Re-exported so the symbol `ChatPage` exported before this extraction stays // importable from here; the implementation lives in `utils/browseCommand` so a // pure test need not pull ChatPage's module graph. @@ -282,7 +283,7 @@ import SubagentProgressBar from './chat/SubagentProgressBar' import TaskProgressBar from './chat/TaskProgressBar' import SidePanel, { CHAT_PANE_MIN_W, sidePanelFillWidth } from './chat/SidePanel' import { useSidePanelDock } from '../hooks/useSidePanelDock' -import { createTurnGrouper, applyRunningState, hasReasoningContent, isReasoningRole, TURN_OPENER_ROLES } from './chat/groupDisplayItems' +import { createTurnGrouper, applyRunningState, hasReasoningContent, REASONING_ROLES, TURN_OPENER_ROLES } from './chat/groupDisplayItems' // Hold-down for the display-layer running latch: a slots broadcast that // catches the agent between tool calls flaps `running` false for well under // a second; only a false that persists longer reflects the turn ending. @@ -504,6 +505,11 @@ export function ChatHeaderMenu({ activeSlot, agent, onReveal, onRename, mode }: * transcript page carries this id back and the bubble is matchable without * relying on content equality (#2845). Shared by the plain send path and the * mid-turn steer path (#6075) so the two cannot drift in id shape. */ +/** ChatPage's tool rows derive their auto-denied state inside ToolCallLine; + * the registry default that reads this set is never reached here. Frozen and + * shared so the per-row context does not allocate. */ +const NO_AUTO_DENIED = new Set() + function mintSendId(): string { return `s-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 8)}` } @@ -7660,96 +7666,36 @@ export default function ChatPage({ mode, embedded, embedMode, popout, noUrlSync const lastTextIdxRef = useRef(lastTextIdx); lastTextIdxRef.current = lastTextIdx const slotStateRef2 = useRef(slotState); slotStateRef2.current = slotState - const renderMessage = useCallback((i: number, m: ChatMessage) => { - // Key identity rules (clientTs preference + streamingβ†’assistant role - // normalization) live in messageRowKey β€” see its doc comment. - const key = messageRowKey(m, i) - // Shared with the wrap gate and fold β€” see hasReasoningContent in - // groupDisplayItems.ts for why there is ONE definition of this condition. - if (hasReasoningContent(m)) return - if (isReasoningRole(m)) return null - if (m.role === 'tool') { - // Skip βœ…/🚫 completion messages β€” completion shown via CircleCheckBig icon - if (!m.content.startsWith('πŸ”§')) return null - // A workflow_run launch renders as a persistent, clickable inline card - // (live status + open-panel affordance) instead of the generic tool pill. - const wfRunId = extractWorkflowRunId(m) - if (wfRunId) return - // Likewise a spawn_run launch: the transient chip above the composer - // drops when the wave ends and only covers the viewed slot, so without - // this the only record of a spawn is a pill folded into "Worked through - // N steps". - const spawnLaunch = extractSpawnRunLaunch(m) - if (spawnLaunch) return - // Animate tools in the trailing group (after last assistant/streaming text) - const isInTrailingGroup = slotStateRef2.current === 'tool_running' && i > lastTextIdxRef.current - // Disclosure identity folds in tool_call_id (#8204) β€” same-tick tool rows - // share the row key, and keying the disclosure map by it made one row's - // expand/collapse hit them all. React key stays the row key on purpose: - // sibling uniqueness is owned by the keyed wrapper, and remounting on a - // key change here would drop measured heights for nothing. - const dKey = toolDisclosureKey(m, key) - return - } - if (m.role === 'file') { - try { - const f = JSON.parse(m.content) - return - } catch { /* fall through to default */ } - } - if (m.role === 'queued') return null - // Auto-nudge turns are machine-facing instruction blobs β€” collapse them to - // a compact chip instead of rendering the whole payload as a chat bubble. - // The Loop button is offered only when this row's own loop is the one still - // bound to the slot, so a historical card never opens a successor loop's - // controls. - if (m.role === 'nudge') { - const ownLoop = nudgeMatchesLoop(m, autoNudgeLoop?.id) - return setAutoNudgeOpen(true) : undefined} /> - } - if (m.kind === 'stop_event' || m.meta?.kind === 'stop_event') return - // A synthetic turn-recovery continuation (tool refusal / stalled turn / - // stalled tool) is machine-facing instruction text. It stays in the - // transcript for auditability, but as a one-line card that names the event - // and the deny pattern rather than a full-width bubble of prompt prose. - if (m.role === 'inject') { - // One shared decision (resolveInjectCard) so this surface and the - // transcript-renderer registry cannot disagree about the same row. It - // returns null for a cron row, for a replay of the user's own words, and - // for a row with no provenance stamp β€” each of which keeps the renderer - // below. Anything positively marked gateway-authored folds into a note - // instead of falling through to a full-width bubble, which is the defect - // this replaces. - const card = resolveInjectCard(m) - if (card) return - } - if (m.role === 'error') return ( - - ) - if (m.role === 'notice') return - if (m.role === 'permission') return null - if (m.role === 'mcp_oauth') { - const banner = renderMcpOAuthMessage(m, connectionsUiOn) - return banner ?
{banner}
: null - } - // An injected workflow completion event renders as a compact status card - // (with the full result folded away) instead of a wall of raw JSON. - if (isWorkflowCompletionMessage(m)) return - // An injected sub-agent completion event is machine-facing prompt text (the - // spawn-discipline instructions are addressed to the model). It renders as a - // compact outcome row with the payload folded away, not as a chat bubble. - if (isSubagentCompletionMessage(m)) return - // A quiet monitor-loop cycle replies with a bare zero-width space - // (U+200B): the content is truthy but renders as nothing, so the row - // would draw as an empty bubble β€” one per quiet cycle, historical - // transcripts included. Skip it; rows carrying file-change chips still - // render (the chips are the content). Same skip as the app-sdk registry. - if (isHiddenInvisibleAssistantRow(m)) return null + // ── Registry-driven row dispatch (chat-core P5-a) ── + // Every transcript row on this page resolves through the SAME renderer + // registry the other surfaces consume (app-sdk/messageRenderers), so a role + // registered once renders everywhere -- the double-wiring defect class + // (`mcp_oauth` shipped wired in app-sdk and raw in the main chat) is closed + // structurally rather than by the parity test alone. ChatPage's chrome (tool + // disclosure state, fork/pin/footer, the error card's Continue, the nudge + // card's Loop button, ...) rides as HOST ENTRIES that reuse the default ids + // they replace, plus a few page-only shape entries. Order inside this array + // is the page's precedence order, unchanged from the if-chain it replaces: + // reasoning, tool, file, nudge, stop_event, inject-recovery, error, notice, + // permission, mcp_oauth, workflow completion, sub-agent completion, hidden + // invisible assistant, then the conversational bubble. Roles none of these + // claim fall to the registry defaults (`undrawn` for queued/system/done and + // the reasoning roles; `tool_lifecycle` for raw wire shapes the store + // normalizes away), and a role NOBODY claims renders as the bubble, which is + // what the if-chain's fall-through did. + // + // Memoized with the deps the old renderMessage carried: UI-state deps + // (chatConfig, linkPreviewsOn, disclosure, pin state, ...) deliberately STAY + // in the array so settled turns re-render with the new behavior, and the + // changed identity is what breaks through memo(TurnBlock). + const { renderers: chatPageRenderers, fallback: bubbleRenderer } = useMemo<{ renderers: readonly MessageRenderer[]; fallback: MessageRenderer }>(() => { + /** The conversational row: user / inject (cron & recovery prose) / assistant. */ + const bubble: MessageRenderer = { + id: 'bubble', + roles: ['user', 'assistant', 'streaming', 'inject'], + render: (m, ctx) => { + const i = ctx.index + const key = ctx.key const isUser = m.role === 'user' const isStreaming = m.role === 'streaming' const isInject = m.role === 'inject' @@ -7847,31 +7793,212 @@ export default function ChatPage({ mode, embedded, embedMode, popout, noUrlSync ) - // dispatch/navigate are stable; handleOpenDiff/handlePlanFromHere are - // memoized callbacks; planTaskId is read when rendering the plan footer / - // apply-plan handler, so it belongs here for correctness. approve/send/ - // dismissApproval are NOT referenced in this renderer (user/approval rows go - // through renderUserContentCb), so they are omitted to keep it stable. - // cursorIsForActiveSlot/slotOldestIndex/handleLoadEarlier belong here: a switch - // back restores the cursor while changing no other dep, stranding Fork shut. - // continuable/interrupted/continuing/lastErrorIdx gate the error card's Continue - // control, so they belong here for the same reason: they are booleans and an int - // (all false/-1 for the whole of a healthy stream, so no per-chunk churn), and - // holding a stale copy is what leaves a superseded failure card offering a - // Continue β€” the exact pair of bugs selectContinuable's doc comment describes. - // handleContinue/handleFolderOpen/handleSpeak/handleApplyPlan cost nothing: each - // one's own dep array is already covered here (handleFolderOpen's is a subset of - // handleFileOpen's), so none can change identity on a render this list survives. - // - // revealAppInPanel is named here rather than excluded: it depends on - // `search.close` (stable) rather than the whole `search` object that - // useMessageSearch rebuilds as a fresh literal every render, so it holds one - // identity and cannot churn this callback β€” or renderTurnItem below it β€” and - // defeat memo(TurnBlock) for settled turns. Excluding it instead would leave it - // captured across a render where the find pane opens, and the stale copy would - // open an app tab behind the still-hidden dock. + }, + } + const renderers = mergeRenderers([ + { + // Shared with the wrap gate and fold -- see hasReasoningContent in + // groupDisplayItems.ts for why there is ONE definition of this condition. + // (A reasoning ROLE without reasoning content falls to the registry's + // `undrawn` default, which is the `isReasoningRole -> null` arm.) + id: 'thinking_block', + roles: ['*'], + match: hasReasoningContent, + render: (m, ctx) => , + }, + { + id: 'tool', + roles: ['tool'], + render: (m, ctx) => { + const i = ctx.index + const key = ctx.key + // Skip βœ…/🚫 completion messages β€” completion shown via CircleCheckBig icon + if (!m.content.startsWith('πŸ”§')) return null + // A workflow_run launch renders as a persistent, clickable inline card + // (live status + open-panel affordance) instead of the generic tool pill. + const wfRunId = extractWorkflowRunId(m) + if (wfRunId) return + // Likewise a spawn_run launch: the transient chip above the composer + // drops when the wave ends and only covers the viewed slot, so without + // this the only record of a spawn is a pill folded into "Worked through + // N steps". + const spawnLaunch = extractSpawnRunLaunch(m) + if (spawnLaunch) return + // Animate tools in the trailing group (after last assistant/streaming text) + const isInTrailingGroup = slotStateRef2.current === 'tool_running' && i > lastTextIdxRef.current + // Disclosure identity folds in tool_call_id (#8204) β€” same-tick tool rows + // share the row key, and keying the disclosure map by it made one row's + // expand/collapse hit them all. React key stays the row key on purpose: + // sibling uniqueness is owned by the keyed wrapper, and remounting on a + // key change here would drop measured heights for nothing. + const dKey = toolDisclosureKey(m, key) + return + }, + }, + { + id: 'file', + roles: ['file'], + render: (m, ctx) => { + const key = ctx.key + try { + const f = JSON.parse(m.content) + return + } catch { /* fall through to default */ } + // An unparseable file row: the if-chain fell through to the bubble. + return bubble.render(m, ctx) + }, + }, + { + // Auto-nudge turns are machine-facing instruction blobs -- collapse them + // to a compact chip instead of rendering the whole payload as a chat + // bubble. The Loop button is offered only when this row's own loop is + // the one still bound to the slot, so a historical card never opens a + // successor loop's controls. + id: 'nudge', + roles: ['nudge'], + render: (m, ctx) => { + const key = ctx.key + const ownLoop = nudgeMatchesLoop(m, autoNudgeLoop?.id) + return setAutoNudgeOpen(true) : undefined} /> + }, + }, + { + id: 'stop_event', + roles: ['*'], + match: m => m.kind === 'stop_event' || m.meta?.kind === 'stop_event', + render: (m, ctx) => , + }, + { + // A synthetic turn-recovery continuation (tool refusal / stalled turn / + // stalled tool) is machine-facing instruction text. It stays in the + // transcript for auditability, but as a one-line card that names the + // event and the deny pattern rather than a full-width bubble of prompt + // prose. An inject row this does not claim is the bubble's. + id: 'recovery_inject', + roles: ['inject'], + match: m => resolveInjectCard(m) != null, + render: (m, ctx) => { + const key = ctx.key + // One shared decision (resolveInjectCard) so this surface and the + // transcript-renderer registry cannot disagree about the same row. It + // returns null for a cron row, for a replay of the user's own words, and + // for a row with no provenance stamp β€” each of which keeps the renderer + // below. Anything positively marked gateway-authored folds into a note + // instead of falling through to a full-width bubble, which is the defect + // this replaces. + const card = resolveInjectCard(m) + if (card) return + return null + }, + }, + { + id: 'error', + roles: ['error'], + render: (m, ctx) => { + const i = ctx.index + const key = ctx.key + return ( + + ) + }, + }, + { id: 'notice', roles: ['notice'], render: (m, ctx) => }, + { + // Approval flow: the permission cards own it; grouped, never a standalone row. + id: 'permission', + roles: ['permission'], + render: () => null, + }, + { + // The page's undrawn set is NARROWER than the SDK default's: reasoning + // roles without reasoning content (the old `isReasoningRole -> null` + // arm) and the queue rail's rows draw nothing here, but `system` / + // `done` -- lifecycle markers this store never carries -- are left + // unclaimed on purpose, so they take the bubble fallback exactly as the + // if-chain's fall-through did rather than vanishing. + id: 'undrawn', + roles: [...REASONING_ROLES, 'queued'], + render: () => null, + }, + { + id: 'mcp_oauth', + roles: ['mcp_oauth'], + render: (m, ctx) => { + const key = ctx.key + const banner = renderMcpOAuthMessage(m, connectionsUiOn) + return banner ?
{banner}
: null + }, + }, + { + // An injected workflow completion event renders as a compact status card + // (with the full result folded away) instead of a wall of raw JSON. + id: 'workflow_completion', + roles: ['*'], + match: isWorkflowCompletionMessage, + render: (m, ctx) => { + const key = ctx.key + return + }, + }, + { + // An injected sub-agent completion event is machine-facing prompt text + // (the spawn-discipline instructions are addressed to the model). It + // renders as a compact outcome row with the payload folded away, not as + // a chat bubble. + id: 'subagent_completion', + roles: ['*'], + match: isSubagentCompletionMessage, + render: (m, ctx) => { + const key = ctx.key + return + }, + }, + { + // A quiet monitor-loop cycle replies with a bare zero-width space + // (U+200B): the content is truthy but renders as nothing, so the row + // would draw as an empty bubble -- one per quiet cycle, historical + // transcripts included. Skip it; rows carrying file-change chips still + // render (the chips are the content). Same skip as the app-sdk registry. + id: 'hidden_invisible_assistant', + roles: ['*'], + match: isHiddenInvisibleAssistantRow, + render: () => null, + }, + bubble, + ]) + return { renderers, fallback: bubble } }, [slotRunning, handleFileOpen, handleArtifactOpen, selectSessionTab, sessionTitles, connected, handleFork, handleQuote, handleAsk, chatConfig, activeSlot, regenerating, handleRegenerate, handleEditResend, slotHasMore, loadingOlder, cursorIsForActiveSlot, slotOldestIndex, handleLoadEarlier, renderUserContentCb, highlightTs, activeSlotTitle, mode, embedded, popout, handleOpenDiff, handlePlanFromHere, planTaskId, artifactPaths, autoNudgeLoop, toolDisclosure, setToolDisclosureFor, linkPreviewsOn, socialShareOn, handleSubagentPanelOpen, isPinned, handleTogglePinForMessage, connectionsUiOn, showRefusedPress, transcriptHot, revealAppInPanel, continuable, interrupted, continuing, lastErrorIdx, handleContinue, handleFolderOpen, handleSpeak, handleApplyPlan, mcpAppPanel]) + const renderMessage = useCallback((i: number, m: ChatMessage) => { + // Key identity rules (clientTs preference + streaming->assistant role + // normalization) live in messageRowKey -- see its doc comment. + const key = messageRowKey(m, i) + const ctx: MessageRenderContext = { + index: i, + messages: messagesRef.current, + running: slotRunning, + key, + onFileOpen: handleFileOpen, + hideCardOwnedOAuth: connectionsUiOn, + autoDeniedIds: NO_AUTO_DENIED, + // Only a registry DEFAULT this page does not override reaches these + // (raw wire-shape tool rows the store normalizes away); the page's own + // entries return keyed elements directly. + wrapper: (children) =>
{children}
, + row: (children) =>
{children}
, + } + const entry = resolveRenderer(m, chatPageRenderers) + // A role nobody claims renders as the conversational bubble -- what the + // if-chain's fall-through did, so an unknown role is visible, never lost. + // By reference: the merged list's tail is an SDK default, not the bubble. + return (entry ?? bubbleRenderer).render(m, ctx) + }, [chatPageRenderers, bubbleRenderer, slotRunning, handleFileOpen, connectionsUiOn]) + // Hoisted out of the row map so every TurnBlock receives the SAME function // identity per render β€” an inline closure there re-created it per row per // render and defeated memo(TurnBlock) even when the turn identity was stable diff --git a/website/src/pages/chat/transcriptRenderers.tsx b/website/src/pages/chat/transcriptRenderers.tsx index 93f12b120c6..131bdb120d7 100644 --- a/website/src/pages/chat/transcriptRenderers.tsx +++ b/website/src/pages/chat/transcriptRenderers.tsx @@ -1,7 +1,12 @@ /** * transcriptRenderers β€” the dashboard's row set for the shared chat transcript. * - * The single-chat surface (ChatPage) draws its rows from a local role chain. + * The single-chat surface (ChatPage) dispatches through the same registry with + * its OWN host list (see `chatPageRenderers` in pages/ChatPage.tsx -- chat-core + * P5-a); the two dashboard host lists share ids (`tool`, `file`, `nudge`, + * `recovery_inject`, `thinking_block`, `error`, `workflow_completion`, + * `subagent_completion`) so they can converge by override, but they are still + * two lists until a P5 follow-up spreads this factory into ChatPage's. * Every OTHER dashboard surface draws through app-sdk/ChatMessageList, whose * default registry is deliberately store-free and therefore renders a WEAKER * transcript: a static pill instead of the live tool line, and nothing at all diff --git a/website/src/test/ChatPage.mcpOAuth.test.tsx b/website/src/test/ChatPage.mcpOAuth.test.tsx index 27553381d0f..5163dcad0ff 100644 --- a/website/src/test/ChatPage.mcpOAuth.test.tsx +++ b/website/src/test/ChatPage.mcpOAuth.test.tsx @@ -1,10 +1,11 @@ /** * Guards the MCP OAuth banner wiring in ChatPage. * - * ChatPage.renderMessage must route messages with role 'mcp_oauth' to - * renderMcpOAuthMessage so the Authorize banner renders inline. If that branch - * (or its import) is dropped, the message falls through to AssistantMessage and - * the raw "πŸ” … requires authentication." text is shown instead of the banner. + * ChatPage's renderer entries must route messages with role 'mcp_oauth' to + * renderMcpOAuthMessage so the Authorize banner renders inline. If that entry + * (or its import) is dropped, the message falls to the registry default, which + * draws the banner inside the page's generic row wrapper instead of as a keyed + * page row; the entry is pinned here so that wiring stays explicit. * * This is a source-contract test: ChatPage's message list is driven by the * custom virtualizer (useVirtualChat), which mounts an empty window under jsdom @@ -28,12 +29,15 @@ describe('ChatPage – MCP OAuth banner wiring', () => { }) it('routes the mcp_oauth message role to the banner renderer', () => { - expect(chatPageSrc).toMatch(/role\s*===\s*['"]mcp_oauth['"]/) + // Since chat-core P5-a the page dispatches rows through the app-sdk + // registry: the banner is the page's `mcp_oauth` HOST ENTRY (same id as + // the registry default it overrides, claiming the role), not an if-branch. + expect(chatPageSrc).toMatch(/id:\s*'mcp_oauth',\s*\n\s*roles:\s*\['mcp_oauth'\]/) expect(chatPageSrc).toMatch(/renderMcpOAuthMessage\s*\(/) }) - it('keeps the banner branch and its renderer call in the same render path', () => { - const idxRole = chatPageSrc.search(/role\s*===\s*['"]mcp_oauth['"]/) + it('keeps the banner entry and its renderer call in the same render path', () => { + const idxRole = chatPageSrc.search(/id:\s*'mcp_oauth',\s*\n\s*roles:\s*\['mcp_oauth'\]/) const idxCall = chatPageSrc.indexOf('renderMcpOAuthMessage(') expect(idxRole).toBeGreaterThanOrEqual(0) expect(idxCall).toBeGreaterThanOrEqual(0) diff --git a/website/src/test/NoticeCard.test.tsx b/website/src/test/NoticeCard.test.tsx index e65fe268d68..2c1508d6b45 100644 --- a/website/src/test/NoticeCard.test.tsx +++ b/website/src/test/NoticeCard.test.tsx @@ -163,7 +163,9 @@ describe('registry wiring', () => { it("ChatPage renders the notice role through NoticeCard, not a hand-rolled box", () => { const here = dirname(fileURLToPath(import.meta.url)) const src = readFileSync(resolve(here, '../pages/ChatPage.tsx'), 'utf8') - expect(src).toMatch(/m\.role === 'notice'.* { }) it('checks for a card BEFORE the generic inject bubble renders', () => { - // The generic `isInject` branch paints any injected text as a full-width - // warning bubble. If the resolver check lands after it, the card is dead - // code and the raw prompt reappears. - const card = src.indexOf('resolveInjectCard(m)') - const generic = src.indexOf("const isInject = m.role === 'inject'") - expect(card).toBeGreaterThanOrEqual(0) - expect(generic).toBeGreaterThanOrEqual(0) - expect(card).toBeLessThan(generic) + // Since chat-core P5-a the page dispatches through the app-sdk registry + // and precedence is the order of its host entries. The generic bubble + // entry (which paints any injected text as a full-width warning bubble) + // is the LAST entry; the `inject_recovery` shape entry must sit before it, + // or the card is dead code and the raw prompt reappears. + const list = src.indexOf('const renderers = mergeRenderers([') + const card = src.indexOf("id: 'recovery_inject'", list) + const generic = src.indexOf('\n bubble,\n ])', list) + expect(list).toBeGreaterThanOrEqual(0) + expect(card).toBeGreaterThan(list) + expect(generic).toBeGreaterThan(card) + expect(src).toMatch(/id: 'recovery_inject',\s*\n\s*roles: \['inject'\],\s*\n\s*match: m => resolveInjectCard\(m\) != null/) }) }) diff --git a/website/src/test/chatRolesParity.contract.test.ts b/website/src/test/chatRolesParity.contract.test.ts index 91dcc19974b..2b88bb35d06 100644 --- a/website/src/test/chatRolesParity.contract.test.ts +++ b/website/src/test/chatRolesParity.contract.test.ts @@ -1,81 +1,71 @@ /** - * Role-parity contract between the two transcript render paths. + * Role-parity contract between ChatPage and the transcript renderer registry. * - * The dashboard renders chat messages through TWO paths: ChatPage's inline - * `renderMessage` if-chain, and the registry in `app-sdk/messageRenderers` - * that every other surface (SideChat, ChatPane, ChatEmbed) consumes via - * ChatMessageList. A role wired in only one path ships a surface where that - * message renders as raw text or not at all β€” `mcp_oauth` shipped exactly - * this way once, wired in app-sdk but not in the main chat. + * Every chat surface renders rows through `app-sdk/messageRenderers`. Until + * chat-core P5-a, ChatPage was the exception: an inline `renderMessage` + * if-chain that had to be kept in step with the registry by hand, and the + * defect class that bought this test -- `mcp_oauth` wired in app-sdk and + * rendered as raw text in the main chat -- lived in that gap. ChatPage now + * DISPATCHES through the registry (`resolveRenderer` over + * `mergeRenderers(chatPageRenderers)`), so a role registered once renders on + * every surface by construction. What is left to guard: * - * Until ChatPage consumes the registry directly (the chat-core extraction's - * later phase), this contract is the guard: every role literal that ChatPage's - * source dispatches on must be CLAIMED by the registry, or be explicitly - * listed here as chrome-only with a reason. Adding a role branch to ChatPage - * without touching either list fails this test. + * 1. The dispatch stays registry-driven: the renderer block in ChatPage + * contains no `if (m.role === '…')` dispatch of its own. + * 2. ChatPage's host entries either OVERRIDE a default (same id, page chrome + * layered on the shared row) or are one of the documented page-only shape + * entries below -- an undocumented id is a fork of the registry in disguise. + * 3. Role literals ChatPage still uses OUTSIDE the renderer (chrome logic: + * footer rules, queue rail, last-error lookup, permission grouping) name + * roles the registry claims, or are allowlisted as chrome with a reason. */ import { describe, it, expect } from 'vitest' import { readFileSync } from 'node:fs' import { resolve } from 'node:path' import { defaultMessageRenderers } from '../app-sdk/messageRenderers' -import { REASONING_ROLES } from '../pages/chat/groupDisplayItems' /** - * Roles ChatPage handles that are deliberately NOT a registry row. Each entry - * must say why it is chrome rather than a transcript row type β€” an entry - * without a reason is a parity gap hiding behind the allowlist. + * Roles ChatPage's CHROME logic names that are deliberately NOT a registry + * row. Each entry must say why -- an entry without a reason is a parity gap + * hiding behind the allowlist. (`queued` and `streaming` left this list with + * P5-a: the registry claims both -- `undrawn` and the assistant entry.) */ const CHROME_ONLY_ROLES: Record = { - // Rendered as the QueueStack card rail above the composer, not as a - // transcript row (the registry deliberately draws nothing for it). - queued: 'composer rail, not a transcript row', - // Approval flow: resolved inline into grouped tool rows; the standalone - // role is chrome that the permission cards own. + // Approval flow: resolved inline into grouped tool rows (GROUPED_ROLES); + // the standalone role is chrome that the permission cards own. ChatPage's + // own `permission` entry draws nothing for the same reason. permission: 'approval cards own it; grouped, never a standalone row', - // Pseudo-role: normalized to `assistant` before dispatch on both paths. - streaming: 'alias of assistant during a live turn', } /** - * Roles only the registry claims, with the reason each is legitimate. These - * arrive on surfaces that read the raw snapshot endpoints (app embeds, side - * sessions), whose payloads carry wire-shape roles ChatPage's normalized - * store never sees. + * ChatPage host entries that do not override a default. Each is a SHAPE entry + * (`roles: ['*']` + `match`) or a deliberately undrawn role, with the reason + * it is page-only rather than a registry default. */ -const REGISTRY_ONLY_ROLES: Record = { - tool_call: 'raw snapshot wire shape; ChatPage store normalizes to tool', - tool_result: 'raw snapshot wire shape; ChatPage store normalizes to tool', - system: 'lifecycle marker in raw snapshots; deliberately undrawn', - done: 'lifecycle marker in raw snapshots; deliberately undrawn', +const PAGE_ONLY_ENTRY_IDS: Record = { + thinking_block: 'ThinkingBlock with page disclosure state; the registry folds reasoning into the group summary (same id as transcriptRenderers)', + recovery_inject: 'RecoveryCard for gateway-authored inject rows; the registry renders inject as prose (resolveInjectCard decides, shared; same id as transcriptRenderers)', + permission: 'undrawn here; the registry leaves it to GROUPED_ROLES', + workflow_completion: 'WorkflowCompletionCard needs page-only session/folder/panel hand-offs', + hidden_invisible_assistant: 'zero-width-space quiet-cycle rows; the registry applies the same skip inside its assistant entry', + bubble: 'the page\'s user / inject / assistant row, with fork, pin, footer, regenerate and search-scope chrome', +} + +const src = readFileSync(resolve(__dirname, '../pages/ChatPage.tsx'), 'utf8') + +function rendererBlock(): string { + const start = src.indexOf('fallback: bubbleRenderer } = useMemo') + const end = src.indexOf('const renderMessage = useCallback', start) + if (start < 0 || end < 0) throw new Error('ChatPage renderer block not found -- did the P5-a dispatch move?') + return src.slice(start, end) } function chatPageRoleLiterals(): Set { - const src = readFileSync(resolve(__dirname, '../pages/ChatPage.tsx'), 'utf8') const roles = new Set() // Both dispatch shapes used in the file: `m.role === 'x'` and - // `messages[i].role === 'x'` (and their !== variants β€” a negative dispatch + // `messages[i].role === 'x'` (and their !== variants -- a negative dispatch // still means the code KNOWS the role). for (const m of src.matchAll(/\.role\s*[!=]==\s*'([a-z_]+)'/g)) roles.add(m[1]) - // Reasoning rows dispatch through the shared predicate (isReasoningRole / - // hasReasoningContent from pages/chat/groupDisplayItems β€” the #6406 - // single-definition consolidation) rather than a role literal. Credit the - // shared list's roles ONLY when ChatPage imports BOTH predicates from the - // shared module AND both dispatch statements are present. This is a textual - // check, not an AST binding check: a comment spelling the exact dispatch - // shape could keep the credit alive β€” accepted, because the companion - // predicate-idiom guard below and the reasoningBurst structural scan bound - // what ChatPage can contain, and losing either import drops the credit - // (the orphan check then reddens). If the dispatch shape is refactored, - // update these patterns. - const importsShared = - /import \{[^}]*\bhasReasoningContent\b[^}]*\} from '\.\/chat\/groupDisplayItems'/.test(src) && - /import \{[^}]*\bisReasoningRole\b[^}]*\} from '\.\/chat\/groupDisplayItems'/.test(src) - const dispatchesReasoning = - /hasReasoningContent\(\w+\)\)\s*return\s* { return roles } -describe('chat role parity (ChatPage renderMessage vs app-sdk registry)', () => { - it('every role ChatPage dispatches on is claimed by the registry or allowlisted as chrome', () => { +function hostEntryIds(): string[] { + return [...rendererBlock().matchAll(/^\s+id: '([a-z_]+)',?$/gm)].map(m => m[1]) +} + +describe('chat role parity (ChatPage consumes the app-sdk registry)', () => { + it('ChatPage dispatches rows through the registry, not an if-chain of its own', () => { + expect(src).toMatch(/import \{[^}]*\bmergeRenderers\b[^}]*\} from '\.\.\/app-sdk\/messageRenderers'/) + expect(src).toMatch(/import \{[^}]*\bresolveRenderer\b[^}]*\} from '\.\.\/app-sdk\/messageRenderers'/) + expect(src).toContain('resolveRenderer(m, chatPageRenderers)') + // Variant flags INSIDE one entry (`const isUser = m.role === 'user'`) are + // fine; a dispatch statement is not -- it would select a row the registry + // never sees. Both spellings of a dispatch are rejected, so a chain cannot + // come back as a `switch`. + expect(rendererBlock()).not.toMatch(/if \(m\.role\s*[!=]==\s*'[a-z_]+'\)/) + expect(rendererBlock()).not.toMatch(/switch \(m\.role\)/) + }) + + it('a role nobody claims falls back to the BUBBLE by reference, never to an SDK default by position', () => { + // mergeRenderers returns [...shapeMatched, ...hostEntries, ...roleKeyedDefaults], + // so the merged list's tail is the SDK `undrawn` default (render: () => null); + // indexing it would make an unregistered role -- the drift this contract + // exists to catch -- vanish from the main chat instead of rendering as text. + expect(rendererBlock()).toContain('return { renderers, fallback: bubble }') + expect(src).toContain('return (entry ?? bubbleRenderer).render(m, ctx)') + expect(src).not.toMatch(/chatPageRenderers\[chatPageRenderers\.length - 1\]/) + // And the page's own `undrawn` override leaves `system` / `done` unclaimed, + // so they keep the if-chain's fall-through instead of the SDK's null. + const undrawn = rendererBlock().match(/id: 'undrawn',\s*\n\s*roles: \[([^\]]*)\]/) + expect(undrawn).not.toBeNull() + expect(undrawn![1]).not.toMatch(/'system'|'done'/) + }) + + it('every ChatPage host entry overrides a default or is a documented page-only entry', () => { + const defaults = new Set(defaultMessageRenderers.map(r => r.id)) + const ids = hostEntryIds() + expect(ids.length).toBeGreaterThan(5) + const undocumented = ids.filter(id => !defaults.has(id) && !(id in PAGE_ONLY_ENTRY_IDS)) + // An entry with a NEW id that also claims a role the defaults render would + // shadow the shared row on this page only -- the fork this contract exists + // to stop. Reuse the default's id to override it, or document why the + // entry is page-only. + expect(undocumented).toEqual([]) + // And the documentation cannot outlive the entry. + const stale = Object.keys(PAGE_ONLY_ENTRY_IDS).filter(id => !ids.includes(id)) + expect(stale).toEqual([]) + }) + + it('every role ChatPage still names is claimed by the registry or allowlisted as chrome', () => { const claimed = registryClaimedRoles() const missing = [...chatPageRoleLiterals()].filter( role => !claimed.has(role) && !(role in CHROME_ONLY_ROLES), ) - // A failure here means a role renders in the main chat but is invisible - // (or raw) in SideChat / ChatPane / ChatEmbed β€” register it in - // app-sdk/messageRenderers, or add it to CHROME_ONLY_ROLES with a reason. + // A role ChatPage's chrome reasons about but no renderer claims would be + // rendered by the page's bubble fallback and by nothing on the other + // surfaces -- register it, or add it to CHROME_ONLY_ROLES with a reason. expect(missing).toEqual([]) }) it('the chrome allowlist carries no stale entries', () => { const known = chatPageRoleLiterals() const stale = Object.keys(CHROME_ONLY_ROLES).filter(role => !known.has(role)) - // An allowlist entry for a role ChatPage no longer mentions is dead - // weight that would silently excuse a future regression β€” remove it. - expect(stale).toEqual([]) - }) - - it('the registry itself only claims roles ChatPage knows (no orphaned surface-only roles)', () => { - const known = chatPageRoleLiterals() - const orphaned = [...registryClaimedRoles()].filter( - role => !known.has(role) && !(role in REGISTRY_ONLY_ROLES), - ) - // A role only the registry knows renders on app surfaces but as raw text - // in the MAIN chat β€” the exact defect class this contract exists to stop - // (that is how mcp_oauth shipped). Wire it into ChatPage too, or record - // why it is a wire-shape role in REGISTRY_ONLY_ROLES. - expect(orphaned).toEqual([]) - }) - - it('the registry-only allowlist carries no stale entries', () => { - const claimed = registryClaimedRoles() - const stale = Object.keys(REGISTRY_ONLY_ROLES).filter(role => !claimed.has(role)) expect(stale).toEqual([]) }) - - it('fails closed: every role dispatch in ChatPage uses the shape the extractor parses', () => { - const src = readFileSync(resolve(__dirname, '../pages/ChatPage.tsx'), 'utf8') - // The extractor understands two idioms: `.role ===/!== ''` - // and the shared reasoning predicates credited above. Any other dispatch - // idiom β€” switch(m.role), a lookup map, comparison against a variable β€” - // would be invisible to the parity checks above, so its mere presence - // fails this contract. If you add one, extend the extractor in this file - // to parse it rather than allowlisting it here. - const comparisons = [...src.matchAll(/\.role\s*[!=]==\s*(\S)/g)] - const nonLiteral = comparisons.filter(m => m[1] !== "'") - expect(nonLiteral.map(m => m[0])).toEqual([]) - expect([...src.matchAll(/switch\s*\([^)]*\.role/g)].map(m => m[0])).toEqual([]) - // Predicate-shaped role dispatch (the #6406 idiom): only the two shared - // reasoning predicates are parsed. A future isRole(...) / hasContent(...) - // helper called in ChatPage is a role dispatch the extractor cannot see, - // so its presence fails here until the extractor learns it. - const predicateCalls = [...src.matchAll(/\b(is[A-Z]\w*Role|has[A-Z]\w*Content)\s*\(/g)].map(m => m[1]) - const unknownPredicates = predicateCalls.filter( - name => name !== 'isReasoningRole' && name !== 'hasReasoningContent', - ) - expect(unknownPredicates).toEqual([]) - }) }) diff --git a/website/src/test/invisibleText.test.ts b/website/src/test/invisibleText.test.ts index fd5aa601336..58dd7e8e62c 100644 --- a/website/src/test/invisibleText.test.ts +++ b/website/src/test/invisibleText.test.ts @@ -81,7 +81,15 @@ describe('ChatPage inline chain consults the skip (source contract)', () => { const src = readFileSync(resolve(__dirname, '../pages/ChatPage.tsx'), 'utf8') it('skips hidden rows before the conversational branch', () => { - expect(src).toMatch(/if \(isHiddenInvisibleAssistantRow\(m\)\) return null/) + // Since chat-core P5-a the page dispatches through the app-sdk registry: + // the skip is a shape entry that draws nothing, ordered before the bubble + // entry (the last one in the host list). + expect(src).toMatch(/id: 'hidden_invisible_assistant',\s*\n\s*roles: \['\*'\],\s*\n\s*match: isHiddenInvisibleAssistantRow,\s*\n\s*render: \(\) => null/) + const list = src.indexOf('const renderers = mergeRenderers([') + const skip = src.indexOf("id: 'hidden_invisible_assistant'", list) + const bubble = src.indexOf('\n bubble,\n ])', list) + expect(skip).toBeGreaterThan(list) + expect(bubble).toBeGreaterThan(skip) }) it('passes over hidden rows in the footer-host scan', () => {