From cee31000a72f05f44bcc988958bd2aa3b5f55da6 Mon Sep 17 00:00:00 2001
From: DavidBabinec <hello@davidbabinec.com>
Date: Fri, 31 Jul 2026 15:00:45 +0200
Subject: [PATCH] fix(import): report loop references the importer cannot
 resolve
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`<instatic-loop data-source-id="posts">` — a table slug where a source id
belongs — imported cleanly. The id was copied verbatim into props,
`site_insert_html` reported success, `verify-docs` found the document intact,
and at publish `loopPrefetch` turned the unregistered source into a
well-formed `{ items: [], totalItems: 0 }`, indistinguishable from "this table
has no rows". The section rendered as nothing and every gate passed, because
the route still returned 200 with a unique title and one h1.

`loopSourceRegistry` has had `has()`/`getOrThrow()` the whole time and nothing
under `htmlImport` ever called either, so the shape `site_list_loop_sources`
documents and the shape the importer accepts were checked against nothing in
common. They now share the registry: the walk reports an unregistered source,
a missing source, and a `data.rows` loop with no table — the second silent
path, where `filters: {}` makes `dataRows` return empty just as quietly.

These are warnings, not rejections. A paste of thirty elements should not be
thrown away over one mistyped attribute, and the importer also serves
arbitrary user-pasted HTML. What matters is that the caller is told at author
time rather than finding an empty section in a screenshot later, so
`site_insert_html` and `site_replace_node_html` return them alongside the ids
they already report. Warnings live on the walk result rather than on
`ImportFragment`, which is the structural shape callers hand back to the store.

Also corrected two descriptions that promised behaviour the composer does not
implement. `<instatic-outlet>` is spliced away on page routes — the page's
content replaces it — so `data-tag` only takes effect on entry routes, and a
template that wants pages to have a `<main>` has to wrap the outlet itself.
The module's own doc comment describes this polymorphism correctly; the tool
description and system prompt did not, which is how a layout ends up with no
main landmark on any public route.

Test: a table slug, a missing id, and a missing table each warn with the fix
in the message; a well-formed loop is silent; the node still imports; several
bad loops in one payload all report.
---
 server/ai/tools/site/systemPrompt.ts          |   2 +-
 server/ai/tools/site/writeTools.ts            |   2 +-
 .../htmlImport/loopReferenceWarnings.test.ts  |  65 +++++++++++
 src/admin/pages/site/agent/executor.ts        |  20 +++-
 src/core/htmlImport/importWarnings.ts         | 103 ++++++++++++++++++
 src/core/htmlImport/walkAndMap.ts             |  28 ++++-
 6 files changed, 209 insertions(+), 11 deletions(-)
 create mode 100644 src/__tests__/htmlImport/loopReferenceWarnings.test.ts
 create mode 100644 src/core/htmlImport/importWarnings.ts

diff --git a/server/ai/tools/site/systemPrompt.ts b/server/ai/tools/site/systemPrompt.ts
index ce1b61315..f2aabed72 100644
--- a/server/ai/tools/site/systemPrompt.ts
+++ b/server/ai/tools/site/systemPrompt.ts
@@ -63,7 +63,7 @@ Loops (repeated CMS/data lists):
 
 Templates (CMS layouts):
 - A template is a document/page that WRAPS other content. Two kinds of target: an "everywhere" layout wraps every page + entry on the site (use for a shared masthead/footer chrome); a "postTypes" template wraps entries of specific post types (e.g. each blog post). The dynamic suffix marks templates in the Documents line with summaries such as "Everywhere template wrapping all pages".
-- The wrapped content flows into a single \`<instatic-outlet data-tag="div">\` you place inside the template's HTML (via site_insert_html) — put it where the page/entry body should appear, with the template's chrome (header/nav/footer) around it. Use the neutral div form when the shared shell already owns \`<main>\`; omit data-tag only when this outlet itself should own the page's main landmark. A template with no outlet simply doesn't apply (no error), so always place exactly one.
+- The wrapped content flows into a single \`<instatic-outlet data-tag="div">\` you place inside the template's HTML (via site_insert_html) — put it where the page/entry body should appear, with the template's chrome (header/nav/footer) around it. On PAGE routes the outlet element itself is not rendered — the page's content is spliced in at that position — so \`data-tag\` there only matters for entry routes, where the outlet stays and wraps the entry body. Author the landmark yourself: wrap the outlet in \`<main>\` in the template's own HTML, or no public page route will have one. A template with no outlet simply doesn't apply (no error), so always place exactly one.
 - Create flow: build the chrome on a page with site_insert_html (including one \`<instatic-outlet>\`), then call site_set_page_template(pageId, target, priority?). For a postTypes target, get valid slugs from site_list_post_types first. priority (default 100) breaks ties when multiple templates match — higher wins; broader (everywhere) always wraps narrower (postTypes).
 - site_clear_page_template(pageId) reverts a template to an ordinary page. Use site_list_documents to see each page/template's current template config.
 
diff --git a/server/ai/tools/site/writeTools.ts b/server/ai/tools/site/writeTools.ts
index c2a0910f3..ac5d1101a 100644
--- a/server/ai/tools/site/writeTools.ts
+++ b/server/ai/tools/site/writeTools.ts
@@ -85,7 +85,7 @@ const insertHtmlTool: AiTool = {
   execution: 'browser',
   requiredCapabilities: SITE_STRUCTURE_CAPS,
   description:
-    'Insert semantic HTML as a subtree of editable nodes under an existing parent. Write structure as HTML (<section>, <h1>, <a>, <button>, <img>, <ul>, ...) and style it with CSS in the same call: put a <style> block in the HTML and/or class= attributes. Custom importer markers: <instatic-loop data-source-id="…" ...> creates a real Loop node (call site_list_loop_sources first for source/table ids and {currentEntry.*} tokens); <instatic-outlet data-tag="div"> creates a neutral template content outlet (omit data-tag only when this outlet should own the page\'s main landmark; use data-custom-tag for a safe custom element). The importer parses every rule — a bare `.foo {}` selector becomes a reusable Selectors-panel class bound to class="foo"; any other selector (`.hero a`, `a:hover`, `nav > li`) becomes an ambient rule. Inline style= attributes land on the node\'s inline styles. To author or edit CSS on its own — pseudo/hover/descendant selectors, or restyling existing rules — use the dedicated site_apply_css tool instead (site_insert_html is for inserting structure). Returns `nodeIds` (the inserted roots) and `created` — every inserted node as { id, moduleId, classes } — so you can target a nested node (e.g. the wrapper you just added) without re-reading the whole tree.',
+    'Insert semantic HTML as a subtree of editable nodes under an existing parent. Write structure as HTML (<section>, <h1>, <a>, <button>, <img>, <ul>, ...) and style it with CSS in the same call: put a <style> block in the HTML and/or class= attributes. Custom importer markers: <instatic-loop data-source-id="…" ...> creates a real Loop node (call site_list_loop_sources first for source/table ids and {currentEntry.*} tokens); <instatic-outlet data-tag="div"> creates a neutral template content outlet. On PAGE routes the outlet element is not rendered at all — the page content is spliced in at its position — so wrap the outlet in <main> yourself if pages should have a main landmark. data-tag only takes effect on ENTRY routes, where the outlet stays and wraps the entry body (use data-custom-tag for a safe custom element). The importer parses every rule — a bare `.foo {}` selector becomes a reusable Selectors-panel class bound to class="foo"; any other selector (`.hero a`, `a:hover`, `nav > li`) becomes an ambient rule. Inline style= attributes land on the node\'s inline styles. To author or edit CSS on its own — pseudo/hover/descendant selectors, or restyling existing rules — use the dedicated site_apply_css tool instead (site_insert_html is for inserting structure). Returns `nodeIds` (the inserted roots) and `created` — every inserted node as { id, moduleId, classes } — so you can target a nested node (e.g. the wrapper you just added) without re-reading the whole tree.',
   inputSchema: InsertHtmlInputSchema,
 }
 
diff --git a/src/__tests__/htmlImport/loopReferenceWarnings.test.ts b/src/__tests__/htmlImport/loopReferenceWarnings.test.ts
new file mode 100644
index 000000000..0bbffa869
--- /dev/null
+++ b/src/__tests__/htmlImport/loopReferenceWarnings.test.ts
@@ -0,0 +1,65 @@
+/**
+ * An unresolvable loop reference is reported at import time.
+ *
+ * `<instatic-loop data-source-id="posts">` — a table slug where a source id
+ * belongs — used to import cleanly. The id was copied verbatim into props, the
+ * document persisted intact, and at publish `loopPrefetch` turned the
+ * unregistered source into a well-formed empty page, indistinguishable from
+ * "this table has no rows". The section rendered as nothing and every gate
+ * passed, because the route still returned 200 with a title and an h1.
+ *
+ * The registry these are checked against is the same one `site_list_loop_sources`
+ * advertises, so the documented shape and the accepted shape finally agree.
+ */
+import { describe, expect, it } from 'bun:test'
+import '@modules/base'
+import '@core/loops/sources'
+import { importHtml } from '@core/htmlImport'
+
+const VALID = '<instatic-loop data-source-id="data.rows" data-table-id="posts"><p>x</p></instatic-loop>'
+
+describe('loop reference warnings', () => {
+  it('warns when the source id is a table slug rather than a source id', () => {
+    const result = importHtml('<instatic-loop data-source-id="posts"><p>x</p></instatic-loop>')
+
+    expect(result.warnings).toHaveLength(1)
+    expect(result.warnings[0]?.kind).toBe('unknown-loop-source')
+    expect(result.warnings[0]?.message).toContain('"posts"')
+    expect(result.warnings[0]?.message).toContain('data.rows')
+  })
+
+  it('warns when a loop has no source id at all', () => {
+    const result = importHtml('<instatic-loop><p>x</p></instatic-loop>')
+
+    expect(result.warnings[0]?.kind).toBe('unknown-loop-source')
+  })
+
+  it('warns when data.rows is missing the table it needs', () => {
+    const result = importHtml('<instatic-loop data-source-id="data.rows"><p>x</p></instatic-loop>')
+
+    expect(result.warnings).toHaveLength(1)
+    expect(result.warnings[0]?.kind).toBe('loop-missing-filter')
+    expect(result.warnings[0]?.message).toContain('data-table-id')
+  })
+
+  it('stays silent on a well-formed loop', () => {
+    expect(importHtml(VALID).warnings).toEqual([])
+  })
+
+  it('still imports the loop node — a warning is not a rejection', () => {
+    const result = importHtml('<instatic-loop data-source-id="posts"><p>x</p></instatic-loop>')
+
+    expect(result.rootIds).toHaveLength(1)
+    expect(Object.values(result.nodes).some((n) => n.moduleId === 'base.loop')).toBe(true)
+  })
+
+  it('reports every bad loop in one payload, and nothing for ordinary markup', () => {
+    const result = importHtml(`
+      <section><h2>Fine</h2><p>Also fine</p></section>
+      <instatic-loop data-source-id="nope-one"><p>a</p></instatic-loop>
+      <instatic-loop data-source-id="nope-two"><p>b</p></instatic-loop>
+    `)
+
+    expect(result.warnings).toHaveLength(2)
+  })
+})
diff --git a/src/admin/pages/site/agent/executor.ts b/src/admin/pages/site/agent/executor.ts
index efa2e1534..bb58de022 100644
--- a/src/admin/pages/site/agent/executor.ts
+++ b/src/admin/pages/site/agent/executor.ts
@@ -217,7 +217,7 @@ function targetNodeIdFromInput(raw: unknown): string | undefined {
  */
 function runInsertHtml(input: InsertHtmlInput): AiToolOutput {
   // (1) Parse and walk the HTML to produce a flat node fragment + any <style> CSS
-  const { nodes, rootIds, styleCss, stripped } = importHtml(input.html)
+  const { nodes, rootIds, styleCss, stripped, warnings } = importHtml(input.html)
   const { rules, conditions } = parseImportedStyleCss(styleCss)
 
   if (rootIds.length === 0) {
@@ -285,7 +285,15 @@ function runInsertHtml(input: InsertHtmlInput): AiToolOutput {
     )
   }
 
-  return aiToolOk({ nodeIds: insertedRootIds, created })
+  // Report references the importer could not resolve. Without this an
+  // `<instatic-loop>` naming a source that does not exist inserts cleanly,
+  // publishes an empty section, and passes every downstream check — the
+  // caller only finds out by looking at the rendered page.
+  return aiToolOk({
+    nodeIds: insertedRootIds,
+    created,
+    ...(warnings.length > 0 ? { warnings: warnings.map((w) => w.message) } : {}),
+  })
 }
 
 /**
@@ -351,7 +359,7 @@ function runReplaceNodeHtml(input: ReplaceNodeHtmlInput): AiToolOutput {
 
   // Parse + validate the payload BEFORE mutating, so an empty / invalid payload
   // never wipes the node's existing children first and then errors out.
-  const { nodes, rootIds, styleCss, stripped } = importHtml(input.html)
+  const { nodes, rootIds, styleCss, stripped, warnings } = importHtml(input.html)
   const { rules, conditions } = parseImportedStyleCss(styleCss)
 
   if (rootIds.length === 0) {
@@ -388,7 +396,11 @@ function runReplaceNodeHtml(input: ReplaceNodeHtmlInput): AiToolOutput {
     return aiToolError(`Node does not accept children: ${input.nodeId}`)
   }
 
-  return aiToolOk({ nodeIds: insertedRootIds })
+  // Same unresolved-reference report as insertHtml.
+  return aiToolOk({
+    nodeIds: insertedRootIds,
+    ...(warnings.length > 0 ? { warnings: warnings.map((w) => w.message) } : {}),
+  })
 }
 
 function runDeleteNode(input: DeleteNodeInput): AiToolOutput {
diff --git a/src/core/htmlImport/importWarnings.ts b/src/core/htmlImport/importWarnings.ts
new file mode 100644
index 000000000..04a68a439
--- /dev/null
+++ b/src/core/htmlImport/importWarnings.ts
@@ -0,0 +1,103 @@
+/**
+ * Import-time diagnostics for authored references the importer cannot verify
+ * by shape alone.
+ *
+ * `<instatic-loop data-source-id="…">` carries a string that must name a
+ * registered loop source. Nothing checked it: the id was copied verbatim into
+ * props, `site_insert_html` reported success, the document persisted intact,
+ * and at publish time `loopPrefetch` turned the unregistered source into a
+ * well-formed `{ items: [], totalItems: 0 }` — indistinguishable from "this
+ * table is empty". The section rendered as nothing, and every gate passed,
+ * because the route still returned 200 with a title and an h1.
+ *
+ * These are warnings rather than hard failures. A paste of thirty elements
+ * should not be rejected wholesale for one mistyped attribute, and the
+ * importer also serves arbitrary user-pasted HTML. What matters is that the
+ * caller is TOLD, at author time, instead of finding an empty section in a
+ * screenshot weeks later.
+ */
+import { loopSourceRegistry } from '@core/loops/registry'
+
+export interface ImportWarning {
+  kind: 'unknown-loop-source' | 'loop-missing-filter'
+  /** Node the warning is about, so a caller can point at it. */
+  nodeId: string
+  message: string
+}
+
+/**
+ * Warnings for one produced node. Returns an empty array for anything that is
+ * not a loop, or when no sources are registered at all — an empty registry
+ * means the host simply has not imported them, and warning on every loop then
+ * would be noise, not signal.
+ */
+export function warningsForNode(
+  nodeId: string,
+  moduleId: string,
+  props: Record<string, unknown>,
+): ImportWarning[] {
+  if (moduleId !== 'base.loop') return []
+  if (loopSourceRegistry.size === 0) return []
+
+  const sourceId = typeof props.sourceId === 'string' ? props.sourceId.trim() : ''
+  if (!sourceId) {
+    return [{
+      kind: 'unknown-loop-source',
+      nodeId,
+      message:
+        'Loop has no data-source-id, so it will render nothing. '
+        + `Valid source ids: ${knownSourceIds()}.`,
+    }]
+  }
+
+  const source = loopSourceRegistry.get(sourceId)
+  if (!source) {
+    return [{
+      kind: 'unknown-loop-source',
+      nodeId,
+      message:
+        `Loop data-source-id "${sourceId}" is not a registered loop source, so it will `
+        + `render nothing. Valid source ids: ${knownSourceIds()}. `
+        + 'A table slug is not a source id — use data-source-id="data.rows" with '
+        + 'data-table-id="<table>".',
+    }]
+  }
+
+  // A source that declares required filters cannot resolve without them:
+  // `data.rows` with no `tableId` returns an empty page exactly like an
+  // unregistered source does.
+  const filters = (props.filters ?? {}) as Record<string, unknown>
+  const missing = requiredFilterKeys(sourceId).filter((key) => {
+    const value = filters[key]
+    return value === undefined || value === null || value === ''
+  })
+  if (missing.length > 0) {
+    return [{
+      kind: 'loop-missing-filter',
+      nodeId,
+      message:
+        `Loop source "${sourceId}" needs ${missing.map((k) => `data-${kebab(k)}`).join(', ')} `
+        + 'and will render nothing without it.',
+    }]
+  }
+
+  return []
+}
+
+/**
+ * Filters a source cannot resolve without. Declared here rather than on the
+ * source because `filterSchema` describes the picker UI and marks nothing as
+ * required; `data.rows` is the only source that hard-returns empty on a
+ * missing filter (see `dataRows.ts`, `if (!opts.tableId) return { items: [] }`).
+ */
+function requiredFilterKeys(sourceId: string): string[] {
+  return sourceId === 'data.rows' ? ['tableId'] : []
+}
+
+function knownSourceIds(): string {
+  return loopSourceRegistry.list().map((source) => `"${source.id}"`).join(', ')
+}
+
+function kebab(value: string): string {
+  return value.replace(/[A-Z]/g, (c) => `-${c.toLowerCase()}`)
+}
diff --git a/src/core/htmlImport/walkAndMap.ts b/src/core/htmlImport/walkAndMap.ts
index a9232407e..2aa470662 100644
--- a/src/core/htmlImport/walkAndMap.ts
+++ b/src/core/htmlImport/walkAndMap.ts
@@ -34,6 +34,7 @@
  */
 
 import type { PageNode } from '@core/page-tree'
+import { warningsForNode, type ImportWarning } from './importWarnings'
 import { createNode } from '@core/page-tree'
 import { registry } from '@core/module-engine'
 import {
@@ -64,6 +65,16 @@ export interface ImportFragment {
   body?: ImportBodyAttributes
 }
 
+/**
+ * What the walk produces: the fragment plus any references it accepted
+ * structurally but could not resolve. Separate from `ImportFragment` because
+ * that is the shape callers hand BACK to the store (`insertImportedNodes`),
+ * and a diagnostic has no place in a structural contract.
+ */
+export interface WalkResult extends ImportFragment {
+  warnings: ImportWarning[]
+}
+
 interface ImportBodyAttributes {
   classIds?: string[]
   inlineStyles?: Record<string, string>
@@ -71,7 +82,7 @@ interface ImportBodyAttributes {
 }
 
 /** The result returned by the convenience entry point importHtml(). */
-export interface ImportResult extends ImportFragment {
+export interface ImportResult extends WalkResult {
   /** Counts of constructs stripped by stripUnsafe(). */
   stripped: StripReport
   /**
@@ -146,6 +157,12 @@ interface WalkContext {
    * collapsed the way normal HTML flow renders it.
    */
   preserveWs: boolean
+  /**
+   * Authored references the importer accepted structurally but cannot resolve
+   * — an unregistered loop source, say. Shared by reference across the whole
+   * walk so nested subtrees report into one list.
+   */
+  warnings: ImportWarning[]
 }
 
 /**
@@ -305,6 +322,7 @@ function processElement(el: Element, ctx: WalkContext): string {
   // from a well-formed baseline.
   const def = registry.getOrThrow(moduleId)
   const node = createNode(moduleId, { ...def.defaults, ...props })
+  ctx.warnings.push(...warningsForNode(node.id, moduleId, node.props))
 
   // Preserve element class *names* verbatim. This layer is registry-agnostic
   // (it has no SiteDocument), so it cannot mint real class ids here. The store
@@ -362,15 +380,15 @@ function processElement(el: Element, ctx: WalkContext): string {
 export function walkAndMap(
   doc: Document,
   inlineStyles: Map<Element, Record<string, string>> = new Map(),
-): ImportFragment {
-  const ctx: WalkContext = { nodes: {}, inlineStyles, preserveWs: false }
+): WalkResult {
+  const ctx: WalkContext = { nodes: {}, inlineStyles, preserveWs: false, warnings: [] }
 
-  if (!doc.body) return { nodes: ctx.nodes, rootIds: [] }
+  if (!doc.body) return { nodes: ctx.nodes, rootIds: [], warnings: [] }
 
   const rootIds = mapChildNodes(doc.body, ctx)
   const body = collectBodyAttributes(doc.body, inlineStyles)
 
-  return { nodes: ctx.nodes, rootIds, ...(body ? { body } : {}) }
+  return { nodes: ctx.nodes, rootIds, warnings: ctx.warnings, ...(body ? { body } : {}) }
 }
 
 /**