diff --git a/packages/editor-ext/src/lib/footnote/footnote-util.derive-id.test.ts b/packages/editor-ext/src/lib/footnote/footnote-util.derive-id.test.ts index 07acab01..96d448ae 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-util.derive-id.test.ts +++ b/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( diff --git a/packages/editor-ext/src/lib/markdown/utils/footnote.marked.ts b/packages/editor-ext/src/lib/markdown/utils/footnote.marked.ts index 58dd27d7..6ad09ece 100644 --- a/packages/editor-ext/src/lib/markdown/utils/footnote.marked.ts +++ b/packages/editor-ext/src/lib/markdown/utils/footnote.marked.ts @@ -12,8 +12,12 @@ import { marked } from "marked"; * single
with one
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]*(.*)$/; diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index 28e5438e..302d2a15 100644 --- a/packages/mcp/build/client.js +++ b/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; } /** diff --git a/packages/mcp/build/lib/footnote-analyze.js b/packages/mcp/build/lib/footnote-analyze.js index 598148cd..0bae93c7 100644 --- a/packages/mcp/build/lib/footnote-analyze.js +++ b/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 } : {}; +} diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 36ee85b6..5a8aaaf7 100644 --- a/packages/mcp/src/client.ts +++ b/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; } diff --git a/packages/mcp/src/lib/footnote-analyze.ts b/packages/mcp/src/lib/footnote-analyze.ts index e6e0d2b9..b259ea00 100644 --- a/packages/mcp/src/lib/footnote-analyze.ts +++ b/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 } : {}; +} diff --git a/packages/mcp/test/unit/footnote-warnings-import.test.mjs b/packages/mcp/test/unit/footnote-warnings-import.test.mjs new file mode 100644 index 00000000..e9abab52 --- /dev/null +++ b/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), {}); +});