Close the push→display validation gap: reject anything the viewer can't render

packages/core/src/validate.ts

@@ -14,6 +14,64 @@
 // since every pushed tree is validated first. No real UI nests anywhere near this deep.
 export const MAX_DEPTH = 256;
 
+/**
+ * Every type the viewer client has a renderer for (registry.ts LOADERS). A push with any other
+ * type used to be stored and 204'd, then surface only as a "[unsupported type: X]" block in the
+ * browser — the pusher never got the feedback loop to fix it (observed in the wild: invented
+ * types like "datatabel", pane types like "Panel"/"Image"). The parity test
+ * (viewer/test/known-parity.test.ts) keeps this list and the client registry in lockstep.
+ */
+export const KNOWN_PUSH_TYPES = [
+  "text", "ansi", "mermaid", "markdown", "panes", "component", "vegalite", "flow", "calltree", "datatable",
+] as const;
+export type KnownPushType = (typeof KNOWN_PUSH_TYPES)[number];
+const KNOWN_TYPE_SET = new Set<string>(KNOWN_PUSH_TYPES);
+const KNOWN_PUSH_TYPES_LIST = KNOWN_PUSH_TYPES.join(", ");
+
+/**
+ * The client component-resolver's whitelist (component-resolver.tsx REGISTRY). A node with any
+ * other `type` renders as a visible "[unknown component: X]" marker — observed in the wild:
+ * "Button", "MapLink", "ListItem" (for "List.Item"). Kept in lockstep by the parity test.
+ */
+export const KNOWN_COMPONENTS = [
+  // layout
+  "Stack", "Group", "Grid", "Grid.Col", "SimpleGrid", "Paper", "Card", "Card.Section", "Box",
+  "Divider", "Center", "ScrollArea",
+  // content
+  "Text", "Title", "Badge", "Pill", "Alert", "List", "List.Item",
+  "Table", "Table.Thead", "Table.Tbody", "Table.Tr", "Table.Th", "Table.Td",
+  "Code", "Blockquote", "Anchor", "Image", "ThemeIcon", "Progress", "RingProgress",
+  "Timeline", "Timeline.Item", "Tabs", "Tabs.List", "Tabs.Tab", "Tabs.Panel",
+  // more display components
+  "Flex", "Space", "Fieldset", "Spoiler", "Kbd", "Mark", "Highlight", "Avatar", "Indicator",
+  "Tooltip", "NumberFormatter", "Notification",
+  "Accordion", "Accordion.Item", "Accordion.Control", "Accordion.Panel",
+  // custom nodes (charts, map, interactive checklist, video card, masonry gallery)
+  "VegaLite", "Map", "Checklist", "Video", "Masonry",
+] as const;
+export type KnownComponent = (typeof KNOWN_COMPONENTS)[number];
+const KNOWN_COMPONENT_SET = new Set<string>(KNOWN_COMPONENTS);
+const KNOWN_COMPONENTS_LIST = KNOWN_COMPONENTS.join(", ");
+// Canonical name by lowercased, dot-stripped form, for "did you mean" (ListItem → List.Item).
+const loose = (s: string): string => s.toLowerCase().replace(/\./g, "");
+const COMPONENT_BY_LOOSE = new Map(KNOWN_COMPONENTS.map((c) => [loose(c), c]));
+
+const componentHint = (type: string): string => {
+  const close = COMPONENT_BY_LOOSE.get(loose(type));
+  return close ? ` (did you mean "${close}"?)` : "";
+};
+
+/** The single definition of "a value the component renderer treats as a component node" — a prop
+ *  value shaped like this WILL be resolved as a node in the browser (and an unknown type rendered
+ *  as a visible marker), so the validator walks exactly the same values — no more (data-shaped
+ *  objects like Vega encodings or Table `data` pass through untouched), no less. The client
+ *  resolver imports this predicate rather than keeping its own copy, so the two can't drift. */
+export const isComponentNodeLike = (v: unknown): v is Record<string, unknown> => {
+  if (typeof v !== "object" || v === null || Array.isArray(v)) return false;
+  const t = (v as Record<string, unknown>).type;
+  return typeof t === "string" && (KNOWN_COMPONENT_SET.has(t) || "props" in v || "children" in v);
+};
+
 /** Validate a `component` node tree. Returns an error message (with a path) or null. */
 export function validateComponentTree(node: unknown, path = "root", depth = 0): string | null {
   if (depth > MAX_DEPTH) return `${path}: component tree too deeply nested (> ${MAX_DEPTH} levels)`;
@@ -28,10 +86,29 @@ export function validateComponentTree(node: unknown, path = "root", depth = 0):
   const n = node as Record<string, unknown>;
   if (typeof n.type !== "string" || n.type.length === 0)
     return `${path}: a component node needs a non-empty string "type" (got ${JSON.stringify(n.type)})`;
+  if (!KNOWN_COMPONENT_SET.has(n.type))
+    return (
+      `${path}: unknown component "${n.type}"${componentHint(n.type)} — the viewer renders it as a ` +
+      `visible "[unknown component]" marker. Supported components: ${KNOWN_COMPONENTS_LIST}`
+    );
   if (n.props !== undefined && (typeof n.props !== "object" || n.props === null || Array.isArray(n.props)))
     return `${path}(${n.type}).props must be an object`;
-  // Only `children` is part of the node tree; `props` values are arbitrary (node-valued props
-  // like icon/label are resolved by the client).
+  // Node-valued props (icon/label/leftSection/… given as {type,...} trees, or arrays of them) are
+  // resolved to elements by the client, so they're validated like children. Anything the client
+  // treats as plain data (no `type`, or an unknown type with no props/children) is left alone.
+  if (n.props) {
+    const props = n.props as Record<string, unknown>;
+    for (const k in props) {
+      const v = props[k];
+      // The all-or-nothing `every` gate on arrays mirrors the client exactly: a mixed array is
+      // treated as plain data there, so it must pass through untouched here too. Recursing on the
+      // whole value (object or array) reuses the tree walker's own array handling for paths/depth.
+      if (isComponentNodeLike(v) || (Array.isArray(v) && v.length > 0 && v.every(isComponentNodeLike))) {
+        const e = validateComponentTree(v, `${path}(${n.type}).props.${k}`, depth);
+        if (e) return e;
+      }
+    }
+  }
   if (n.children !== undefined) return validateComponentTree(n.children, `${path}/${n.type}`, depth + 1);
   return null;
 }
@@ -211,13 +288,26 @@ export function mermaidErrors(text: string): string[] {
   return errors;
 }
 
+// A Vega-Lite spec draws something only if it has a view-defining key; without one, vega-embed
+// throws in the browser ("vega-lite render error") after the server already said 204.
+const VEGA_VIEW_KEYS = ["mark", "layer", "facet", "hconcat", "vconcat", "concat", "repeat", "spec"];
+// The JSON-content types validateContent inspects structurally (everything renderable except the
+// freeform text types and mermaid, which is linted as source text above).
+const STRUCTURED_TYPES = new Set(["component", "panes", "flow", "vegalite", "calltree", "datatable"]);
+
 /** Validate `content` for a given push `type`. Returns an error message or null (valid / freeform).
  *  `depth` bounds nested-panes recursion (a pane's content can itself be a `panes` payload). */
 export function validateContent(type: string, content: string, depth = 0): string | null {
+  // The viewer can only display types it has a renderer for — anything else would be stored,
+  // 204'd, and then break in the browser with no feedback to the pusher.
+  if (!KNOWN_TYPE_SET.has(type))
+    return (
+      `unknown type "${type}" — the viewer has no renderer for it and would show a ` +
+      `"[unsupported type]" block. Supported types: ${KNOWN_PUSH_TYPES_LIST}`
+    );
   // mermaid is NOT freeform — validate against the renderable subset (header + supported constructs).
   if (type === "mermaid") return mermaidErrors(content)[0] ?? null;
-  if (type !== "component" && type !== "panes" && type !== "flow" && type !== "vegalite" && type !== "calltree" && type !== "datatable")
-    return null;
+  if (!STRUCTURED_TYPES.has(type)) return null; // text/ansi/markdown are genuinely freeform
   if (depth > MAX_DEPTH) return `panes nested too deeply (> ${MAX_DEPTH} levels)`;
   let v: unknown;
   try {
@@ -226,22 +316,59 @@ export function validateContent(type: string, content: string, depth = 0): strin
     return `invalid ${type} JSON: ${(e as Error).message}`;
   }
   if (type === "component") return validateComponentTree(v);
-  if (type === "vegalite") return isObj(v) ? null : "vegalite content must be a Vega-Lite spec object";
+  if (type === "vegalite") {
+    if (!isObj(v)) return "vegalite content must be a Vega-Lite spec object";
+    if (!VEGA_VIEW_KEYS.some((k) => v[k] !== undefined))
+      return (
+        'vegalite spec has nothing to draw — it needs "mark" (or layer/facet/concat/repeat/spec), ' +
+        'e.g. { "mark": "bar", "data": { "values": [...] }, "encoding": {...} }'
+      );
+    return null;
+  }
   if (type === "flow") {
     if (!isObj(v)) return "flow content must be an object { nodes, edges, ... }";
-    // `nodes`/`edges` are optional, but when present each entry must be a non-null object: the
-    // layout/geometry pass dereferences `n.data` / `n.group` / `e.source` and a `null` or primitive
-    // entry would otherwise throw deep inside the renderer as an opaque HTTP 500 instead of a
-    // path-pointed 400 here. (Keys beyond that stay lenient — the renderer defaults them safely.)
+    // `nodes`/`edges` are optional, but when present each entry must be an object with a usable
+    // identity: React Flow needs a unique string `id` per node, and an edge whose source/target
+    // doesn't name an existing node is silently dropped from the drawing — the pushed graph looks
+    // subtly wrong with no feedback. All of these are authoring bugs, so they're path-pointed 400s.
+    // (Keys beyond id/source/target/parentId stay lenient — the renderer defaults them safely.)
+    const ids = new Set<string>();
     if (v.nodes !== undefined) {
       if (!Array.isArray(v.nodes)) return "flow.nodes must be an array";
-      for (let i = 0; i < v.nodes.length; i++)
-        if (!isObj(v.nodes[i])) return `flow.nodes[${i}] must be an object`;
+      let hasParentIds = false;
+      for (let i = 0; i < v.nodes.length; i++) {
+        const n = v.nodes[i];
+        if (!isObj(n)) return `flow.nodes[${i}] must be an object`;
+        if (typeof n.id !== "string" || n.id.length === 0)
+          return `flow.nodes[${i}].id must be a non-empty string (got ${JSON.stringify(n.id)})`;
+        if (ids.has(n.id)) return `flow.nodes[${i}].id "${n.id}" is a duplicate — node ids must be unique`;
+        ids.add(n.id);
+        if (n.parentId !== undefined) hasParentIds = true;
+      }
+      // Second pass (only when needed) so a parent declared after its child still resolves.
+      if (hasParentIds) {
+        for (let i = 0; i < v.nodes.length; i++) {
+          const n = v.nodes[i] as Record<string, unknown>;
+          if (n.parentId !== undefined && (typeof n.parentId !== "string" || !ids.has(n.parentId)))
+            return `flow.nodes[${i}].parentId ${JSON.stringify(n.parentId)} does not match any node id`;
+        }
+      }
     }
     if (v.edges !== undefined) {
       if (!Array.isArray(v.edges)) return "flow.edges must be an array";
-      for (let i = 0; i < v.edges.length; i++)
-        if (!isObj(v.edges[i])) return `flow.edges[${i}] must be an object`;
+      const endpointError = (i: number, k: string, ep: unknown): string | null => {
+        if (typeof ep !== "string" || ep.length === 0)
+          return `flow.edges[${i}].${k} must be a non-empty string (a node id, got ${JSON.stringify(ep)})`;
+        if (!ids.has(ep))
+          return `flow.edges[${i}].${k} "${ep}" does not match any node id — the renderer would silently drop this edge`;
+        return null;
+      };
+      for (let i = 0; i < v.edges.length; i++) {
+        const e = v.edges[i];
+        if (!isObj(e)) return `flow.edges[${i}] must be an object`;
+        const err = endpointError(i, "source", e.source) ?? endpointError(i, "target", e.target);
+        if (err) return err;
+      }
     }
     return null;
   }

packages/viewer/e2e/stress-cases/manifest.json

@@ -47,7 +47,7 @@
   {
     "name": "flow_dangling",
     "type": "flow",
-    "expect": "render"
+    "expect": "reject"
   },
   {
     "name": "flow_selfloop",
@@ -72,7 +72,7 @@
   {
     "name": "component_unknown",
     "type": "component",
-    "expect": "render"
+    "expect": "reject"
   },
   {
     "name": "component_longtext",

packages/viewer/e2e/stress.e2e.mjs

@@ -60,14 +60,18 @@ try {
       rows.push({ ...m, pass, stuck: false, signal: false, errBlock: false, errs: 0, note: pass ? "" : `push=${pushStatus[m.name]}` });
       continue;
     }
-    if (pushStatus[m.name] !== 204) {
+    // 204 = stored clean; 200 = stored WITH non-fatal readability findings (geometry lint) —
+    // both mean the board exists and must render.
+    if (pushStatus[m.name] !== 204 && pushStatus[m.name] !== 200) {
       rows.push({ ...m, pass: false, stuck: false, signal: false, errBlock: false, errs: 0, note: `unexpected push reject (${pushStatus[m.name]})` });
       continue;
     }
     const before = errors.length;
     let stuck = false, signal = false, errBlock = false, note = "";
     try {
-      await p.locator("#scopes button", { hasText: `stress/${m.name}` }).click();
+      // Match on the title attribute (like rich.e2e): the sidebar renders project/agent as
+      // separate spans, so the button's inner text no longer contains the literal "proj/agent".
+      await p.locator(`#scopes button[title="stress/${m.name}"]`).click();
       await p.waitForFunction(
         ({ sig, re }) => {
           const d = document.querySelector("#diagram");

packages/viewer/src/client/registry.ts

@@ -1,5 +1,6 @@
 import type { Mount, MountResult } from "./renderers/types.js";
 import type { PatchOp } from "../flow-patch.js";
+import type { KnownPushType } from "@ivanmkc/termchart-core";
 import { unsupportedBlock, errorBlock } from "./renderers/errors.js";
 
 export type { Mount, MountResult } from "./renderers/types.js";
@@ -8,8 +9,11 @@ export type { Mount, MountResult } from "./renderers/types.js";
  * Lazy renderer loaders — one dynamic import() per type. esbuild code-splits each into
  * its own chunk, so heavy libraries (mermaid, React/Mantine, vega, React Flow) load only
  * when a payload of that type first appears. Add a type here + a module under renderers/.
+ * Keyed by core's KnownPushType (what the server validator accepts), so adding a renderer
+ * without whitelisting it — or vice versa — is a compile error; the known-parity test
+ * double-checks the same invariant at the JS level.
  */
-const LOADERS: Record<string, () => Promise<{ mount: Mount }>> = {
+const LOADERS: Record<KnownPushType, () => Promise<{ mount: Mount }>> = {
   text: () => import("./renderers/text.js"),
   ansi: () => import("./renderers/text.js"),
   mermaid: () => import("./renderers/mermaid.js"),
@@ -80,7 +84,7 @@ export async function renderInto(
   teardownTree(el);
   el.replaceChildren();
 
-  const loader = LOADERS[type];
+  const loader = (LOADERS as Record<string, () => Promise<{ mount: Mount }>>)[type];
   if (!loader) {
     el.innerHTML = unsupportedBlock(type, content);
     return { selfSizing: false };

packages/viewer/src/client/renderers/component-resolver.tsx

@@ -6,6 +6,7 @@ import {
   Flex, Space, Fieldset, Spoiler, Kbd, Mark, Highlight, Avatar, Indicator, Tooltip,
   NumberFormatter, Notification, Accordion,
 } from "@mantine/core";
+import { isComponentNodeLike, MAX_DEPTH, type KnownComponent } from "@ivanmkc/termchart-core";
 import { VegaLite } from "./vega-node.js";
 import { Map } from "./map-node.js";
 import { Checklist } from "./checklist.js";
@@ -22,9 +23,12 @@ export interface ComponentNode {
  * Curated whitelist of Mantine components the agent may use. Bounds the chunk and the
  * surface; compound names (e.g. "Grid.Col") map to subcomponents. Charts come from the
  * `VegaLite` node (added once the vega renderer exists) and the standalone `vegalite` type.
+ * `satisfies` ties the key set to core's KNOWN_COMPONENTS (what the server validator accepts),
+ * so a missing or extra entry on either side is a compile error, not a runtime surprise; the
+ * known-parity test double-checks the same invariant at the JS level.
  */
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-const REGISTRY: Record<string, ComponentType<any>> = {
+const REGISTRY = {
   // layout
   Stack, Group, Grid, "Grid.Col": Grid.Col, SimpleGrid, Paper, Card,
   "Card.Section": Card.Section, Box, Divider, Center, ScrollArea,
@@ -50,33 +54,23 @@ const REGISTRY: Record<string, ComponentType<any>> = {
   Video,
   // masonry flow (Pinterest/Flipboard) — auto-arranging multi-column cascade; optional swimlanes
   Masonry,
-};
-
-/** Register additional components (e.g. VegaLite once the vega renderer is available). */
-export function registerComponent(name: string, comp: ComponentType<unknown>): void {
-  REGISTRY[name] = comp;
-}
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+} satisfies Record<KnownComponent, ComponentType<any>>;
 
 /** All registered component names — for tests / introspection. */
 export const componentNames = (): string[] => Object.keys(REGISTRY);
 
 /** True when a value is itself a component node — e.g. an `icon` / `label` / `leftSection`
  *  prop given as a `{type,...}` tree. Such props must be resolved to elements, not passed to
  *  React as raw objects (which throws "Objects are not valid as a React child"). Data-shaped
- *  objects (a Vega spec, `style`, Table `data`) have no whitelisted `type` and are left alone. */
-function isComponentNode(v: unknown): v is ComponentNode {
-  return (
-    typeof v === "object" &&
-    v !== null &&
-    !Array.isArray(v) &&
-    typeof (v as ComponentNode).type === "string" &&
-    (REGISTRY[(v as ComponentNode).type] !== undefined || "props" in v || "children" in v)
-  );
-}
+ *  objects (a Vega spec, `style`, Table `data`) have no whitelisted `type` and are left alone.
+ *  The predicate itself lives in core — the SAME one the push validator walks — so the set of
+ *  values resolved here and the set validated at push time cannot drift. */
+const isComponentNode = (v: unknown): v is ComponentNode => isComponentNodeLike(v);
 
-// Bound recursion (matches the server validator's MAX_DEPTH) so a pathological tree degrades to
+// Bound recursion (the server validator's own cap, imported) so a pathological tree degrades to
 // a marker instead of overflowing the stack. Pushed trees are validated first, so this is a backstop.
-const RESOLVE_MAX_DEPTH = 256;
+const RESOLVE_MAX_DEPTH = MAX_DEPTH;
 
 // URL-valued props whose value must not carry an executable scheme. Pushed content is nominally
 // token-gated, but a read-only /s/<token>/ share viewer renders it too, so a `javascript:`/`data:`
@@ -133,7 +127,8 @@ export function resolve(node: unknown, key?: string | number, depth = 0): ReactN
   if (typeof node !== "object") return null;
 
   const { type, props, children } = node as ComponentNode;
-  const Comp = REGISTRY[type];
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const Comp = (REGISTRY as Record<string, ComponentType<any>>)[type];
   if (!Comp) {
     return (
       <pre className="diagram-pre" key={key}>{`[unknown component: ${type}]`}</pre>

packages/viewer/test/known-parity.test.ts

@@ -0,0 +1,18 @@
+// @vitest-environment happy-dom
+// Parity guards: the server-side whitelists in @ivanmkc/termchart-core must exactly match what the
+// client can actually render. If a renderer type or a whitelisted component is added/removed on one
+// side without the other, pushes would be rejected for renderable content (validator too strict) or
+// stored and broken in the UI (validator too loose). These tests fail the build instead.
+import { describe, expect, it } from "vitest";
+import { KNOWN_PUSH_TYPES, KNOWN_COMPONENTS } from "@ivanmkc/termchart-core";
+import { KNOWN_TYPES } from "../src/client/registry.js";
+import { componentNames } from "../src/client/renderers/component-resolver.js";
+
+describe("core ↔ client whitelist parity", () => {
+  it("KNOWN_PUSH_TYPES matches the client renderer registry exactly", () => {
+    expect([...KNOWN_PUSH_TYPES].sort()).toEqual([...KNOWN_TYPES].sort());
+  });
+  it("KNOWN_COMPONENTS matches the client component registry exactly", () => {
+    expect([...KNOWN_COMPONENTS].sort()).toEqual([...componentNames()].sort());
+  });
+});