Merge pull request 'test(footnotes): cover footnoteWarnings import plumbing + doc fixes (#169 second review)' (#171) from fix/footnote-review-1227-followup into develop

Reviewed-on: #171

packages/editor-ext/src/lib/footnote/footnote-util.derive-id.test.ts

@@ -52,7 +52,7 @@ function singleLetterSuffixes(): string[] {
   return Array.from({ length: 25 }, (_, i) => String.fromCharCode(98 + i));
 }
 
-describe("deriveFootnoteId golden table (cross-package drift guard)", () => {
+describe("deriveFootnoteId golden table (deterministic-scheme pin)", () => {
   for (const row of DERIVE_GOLDEN) {
     it(`derive("${row.originalId}", ${row.occurrence}, {${row.taken.join(",")}}) === "${row.expected}" — ${row.why}`, () => {
       const got = deriveFootnoteId(

packages/editor-ext/src/lib/markdown/utils/footnote.marked.ts

@@ -12,8 +12,12 @@ import { marked } from "marked";
  *    single <section data-footnotes> with one <div data-footnote-def> per
  *    definition, so the round-trip rebuilds footnotesList + footnoteDefinition.
  *
- * Only definitions that have a matching reference are emitted (and vice-versa
- * the sync plugin fills any gaps on the editor side), keeping the output valid.
+ * Every FIRST definition line is emitted — duplicate ids are first-wins (the
+ * rest are dropped, and surfaced via analyzeFootnotes), and reference markers are
+ * left untouched so repeated `[^a]` references reuse the one footnote (#166).
+ * Orphan definitions (no matching reference) are still emitted here; the editor's
+ * sync plugin reconciles the final reference/definition set (drops orphans,
+ * synthesizes a single empty definition for a reference that lacks one).
  */
 
 const DEFINITION_RE = /^\[\^([^\]\s]+)\]:[ \t]*(.*)$/;

packages/mcp/build/client.js

@@ -9,7 +9,7 @@ import WebSocket from "ws";
 import { convertProseMirrorToMarkdown } from "./lib/markdown-converter.js";
 import { updatePageContentRealtime, replacePageContent, markdownToProseMirror, mutatePageContent, buildCollabWsUrl, assertYjsEncodable, } from "./lib/collaboration.js";
 import { docmostExtensions } from "./lib/docmost-schema.js";
-import { analyzeFootnotes } from "./lib/footnote-analyze.js";
+import { footnoteWarningsField } from "./lib/footnote-analyze.js";
 import { buildPageTree } from "./lib/tree.js";
 import { serializeDocmostMarkdown, parseDocmostMarkdown, } from "./lib/markdown-document.js";
 import { replaceNodeById, deleteNodeById, insertNodeRelative, buildOutline, getNodeByRef, readTable, insertTableRow, deleteTableRow, updateTableCell, } from "./lib/node-ops.js";
@@ -820,8 +820,7 @@ export class DocmostClient {
         const page = await this.getPage(newPageId);
         // Surface non-fatal footnote problems (dangling refs, empty/duplicate
         // definitions, markers in tables) so the agent can fix its markup (#166).
-        const { warnings } = analyzeFootnotes(content);
-        return warnings.length > 0 ? { ...page, footnoteWarnings: warnings } : page;
+        return { ...page, ...footnoteWarningsField(content) };
     }
     /**
      * Update a page's content from markdown and optionally its title.
@@ -851,7 +850,6 @@ export class DocmostClient {
             }
             throw new Error(`Failed to update page content: ${error.message}`);
         }
-        const { warnings } = analyzeFootnotes(content);
         return {
             success: true,
             modified: true,
@@ -859,7 +857,7 @@ export class DocmostClient {
             pageId: pageId,
             verify: mutation.verify,
             // Non-fatal footnote diagnostics (#166); omitted when there are none.
-            ...(warnings.length > 0 ? { footnoteWarnings: warnings } : {}),
+            ...footnoteWarningsField(content),
         };
     }
     /**
@@ -1129,11 +1127,11 @@ export class DocmostClient {
         if (meta?.pageId && meta.pageId !== pageId) {
             result.warning = `File was exported from page ${meta.pageId} but is being imported into ${pageId}.`;
         }
-        // Non-fatal footnote diagnostics (#166), analyzed on the body (definitions
-        // and references live there, not in the front-matter/comments sections).
-        const { warnings } = analyzeFootnotes(body);
-        if (warnings.length > 0)
-            result.footnoteWarnings = warnings;
+        // Non-fatal footnote diagnostics (#166), analyzed on the BODY (the part after
+        // the docmost:meta / docmost:comments blocks) — so a `[^x]`-like token inside
+        // those JSON blocks never produces a false warning, while real markers in the
+        // body do. `body` comes from parseDocmostMarkdown(fullMarkdown) above.
+        Object.assign(result, footnoteWarningsField(body));
         return result;
     }
     /**

packages/mcp/build/lib/footnote-analyze.js

@@ -89,3 +89,13 @@ export function analyzeFootnotes(markdown) {
         warnings,
     };
 }
+/**
+ * The optional `footnoteWarnings` field for a page-write tool result: present
+ * (with the warning lines) only when `markdown` has footnote problems, omitted
+ * otherwise. One helper so all three call sites (create/update/import) attach the
+ * field identically. Spread into the result: `{ ...result, ...footnoteWarningsField(text) }`.
+ */
+export function footnoteWarningsField(markdown) {
+    const { warnings } = analyzeFootnotes(markdown);
+    return warnings.length > 0 ? { footnoteWarnings: warnings } : {};
+}

packages/mcp/src/client.ts

@@ -23,7 +23,7 @@ import {
   MutationResult,
 } from "./lib/collaboration.js";
 import { docmostExtensions } from "./lib/docmost-schema.js";
-import { analyzeFootnotes } from "./lib/footnote-analyze.js";
+import { footnoteWarningsField } from "./lib/footnote-analyze.js";
 import { buildPageTree } from "./lib/tree.js";
 import {
   serializeDocmostMarkdown,
@@ -1058,8 +1058,7 @@ export class DocmostClient {
     const page = await this.getPage(newPageId);
     // Surface non-fatal footnote problems (dangling refs, empty/duplicate
     // definitions, markers in tables) so the agent can fix its markup (#166).
-    const { warnings } = analyzeFootnotes(content);
-    return warnings.length > 0 ? { ...page, footnoteWarnings: warnings } : page;
+    return { ...page, ...footnoteWarningsField(content) };
   }
 
   /**
@@ -1100,7 +1099,6 @@ export class DocmostClient {
       throw new Error(`Failed to update page content: ${error.message}`);
     }
 
-    const { warnings } = analyzeFootnotes(content);
     return {
       success: true,
       modified: true,
@@ -1108,7 +1106,7 @@ export class DocmostClient {
       pageId: pageId,
       verify: mutation.verify,
       // Non-fatal footnote diagnostics (#166); omitted when there are none.
-      ...(warnings.length > 0 ? { footnoteWarnings: warnings } : {}),
+      ...footnoteWarningsField(content),
     };
   }
 
@@ -1424,10 +1422,11 @@ export class DocmostClient {
     if (meta?.pageId && meta.pageId !== pageId) {
       result.warning = `File was exported from page ${meta.pageId} but is being imported into ${pageId}.`;
     }
-    // Non-fatal footnote diagnostics (#166), analyzed on the body (definitions
-    // and references live there, not in the front-matter/comments sections).
-    const { warnings } = analyzeFootnotes(body);
-    if (warnings.length > 0) result.footnoteWarnings = warnings;
+    // Non-fatal footnote diagnostics (#166), analyzed on the BODY (the part after
+    // the docmost:meta / docmost:comments blocks) — so a `[^x]`-like token inside
+    // those JSON blocks never produces a false warning, while real markers in the
+    // body do. `body` comes from parseDocmostMarkdown(fullMarkdown) above.
+    Object.assign(result, footnoteWarningsField(body));
     return result;
   }
 

packages/mcp/src/lib/footnote-analyze.ts

@@ -114,3 +114,16 @@ export function analyzeFootnotes(markdown: string): FootnoteDiagnostics {
     warnings,
   };
 }
+
+/**
+ * The optional `footnoteWarnings` field for a page-write tool result: present
+ * (with the warning lines) only when `markdown` has footnote problems, omitted
+ * otherwise. One helper so all three call sites (create/update/import) attach the
+ * field identically. Spread into the result: `{ ...result, ...footnoteWarningsField(text) }`.
+ */
+export function footnoteWarningsField(markdown: string): {
+  footnoteWarnings?: string[];
+} {
+  const { warnings } = analyzeFootnotes(markdown);
+  return warnings.length > 0 ? { footnoteWarnings: warnings } : {};
+}

packages/mcp/test/unit/footnote-warnings-import.test.mjs

@@ -0,0 +1,63 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+
+import {
+  analyzeFootnotes,
+  footnoteWarningsField,
+} from "../../build/lib/footnote-analyze.js";
+import {
+  serializeDocmostMarkdown,
+  parseDocmostMarkdown,
+} from "../../build/lib/markdown-document.js";
+
+// Pins the footnoteWarnings PLUMBING contract (#169 review): the field is
+// present only on problems and omitted on clean input, AND `import_page_markdown`
+// analyzes the BODY (after the docmost:meta / docmost:comments blocks) — so a
+// footnote-like token inside those JSON blocks never warns, while a real marker
+// in the body does. importPageMarkdown does exactly
+// `footnoteWarningsField(parseDocmostMarkdown(full).body)` over a collab socket
+// this harness does not stand up, so we test the same pure composition directly.
+
+test("footnoteWarningsField is present on problems and omitted on clean input", () => {
+  const problem = footnoteWarningsField("See[^missing].\n\n[^a]: defined");
+  assert.ok(Array.isArray(problem.footnoteWarnings));
+  assert.match(problem.footnoteWarnings.join("\n"), /no matching definition/);
+
+  const clean = footnoteWarningsField("A[^a] and reuse[^a].\n\n[^a]: fine");
+  assert.deepEqual(clean, {}); // no key at all on clean input
+});
+
+test("import analyzes the BODY only — tokens inside meta/comments never warn", () => {
+  // meta + comments JSON carry `[^metaonly]` / `[^commentonly]`-looking text; the
+  // BODY has a genuinely dangling `[^bodyref]`.
+  const full = serializeDocmostMarkdown(
+    { pageId: "p1", note: "front-matter mentions [^metaonly] in text" },
+    "Body with a dangling[^bodyref] marker.",
+    [{ id: "c1", content: "a comment that says [^commentonly]" }],
+  );
+
+  const { body } = parseDocmostMarkdown(full);
+  // Sanity: the meta/comments markers are NOT in the parsed body.
+  assert.ok(!body.includes("[^metaonly]"));
+  assert.ok(!body.includes("[^commentonly]"));
+
+  const field = footnoteWarningsField(body);
+  const joined = (field.footnoteWarnings ?? []).join("\n");
+  // ONLY the body's dangling reference is flagged.
+  assert.match(joined, /\[\^bodyref\]/);
+  assert.ok(!joined.includes("metaonly"));
+  assert.ok(!joined.includes("commentonly"));
+
+  // Cross-check against analyzeFootnotes directly (same composition the importer uses).
+  assert.deepEqual(analyzeFootnotes(body).danglingReferences, ["bodyref"]);
+});
+
+test("import on a clean body yields no footnoteWarnings field", () => {
+  const full = serializeDocmostMarkdown(
+    { pageId: "p1" },
+    "Clean body[^a] reusing[^a].\n\n[^a]: ok",
+    [],
+  );
+  const { body } = parseDocmostMarkdown(full);
+  assert.deepEqual(footnoteWarningsField(body), {});
+});