diff --git a/CHANGELOG.md b/CHANGELOG.md index 60cf108e..47ce54f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -394,6 +394,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 anchor mark, silently dropping bold/italic/code/link on the affected run; the prevailing formatting of the replaced run is now carried onto the applied text. (#496) +- **Markdown round-trips no longer silently drop a line that opens with a block + trigger.** When a document is exported to Markdown and re-imported (git-sync + stabilize, agent writes), a paragraph or continuation line (after a hard break) + that begins with a block marker — an ATX heading `#`, a blockquote/callout `>`, + a list marker (`-`/`*`/`+`/`N.`/`N)`), a code fence, a table `|`, a thematic + break (`---`), or a setext underline (`--`, `----`, or a lone `=`) — is now + backslash-escaped so it round-trips as text instead of being re-parsed into a + heading/list/quote/rule and losing its content. Front-matter stripping is + scoped to the import path only. (#493) - **The server no longer runs out of heap during long autonomous agent runs.** A new pnpm patch on `ai@6.0.134` stops the SDK from building a cumulative snapshot of the ENTIRE turn text on every streamed text-delta when no output diff --git a/apps/client/src/features/editor/gitmost/gitmost-recording.test.ts b/apps/client/src/features/editor/gitmost/gitmost-recording.test.ts index 1f5feef8..f357b070 100644 --- a/apps/client/src/features/editor/gitmost/gitmost-recording.test.ts +++ b/apps/client/src/features/editor/gitmost/gitmost-recording.test.ts @@ -9,7 +9,7 @@ import { Italic } from "@tiptap/extension-italic"; import { Link } from "@tiptap/extension-link"; import { gitmostInsertTranscriptIntoEditor } from "./gitmost-recording.ts"; -const ZWSP = "​"; // U+200B, the helper's block-trigger neutralizer +const ZWSP = "​"; // U+200B — asserted ABSENT (the block-escape lives in the serializer now) /** * #377 — the web-side bridge must append the native host's transcript below the @@ -18,8 +18,9 @@ const ZWSP = "​"; // U+200B, the helper's block-trigger neutralizer * regression would be caught), asserting the resulting document rather than * mocking the editor: transcript present -> "Transcript" heading + one paragraph * per non-empty line; content is inserted as LITERAL TEXT (no HTML/markdown - * parsing); col-0 markdown block triggers are neutralized so git-sync keeps them - * paragraphs; absent/empty/non-string -> no-op. + * parsing); col-0 markdown block triggers are stored verbatim (the git-sync + * serializer block-escapes them, so no client-side ZWSP is needed); + * absent/empty/non-string -> no-op. */ describe("gitmostInsertTranscriptIntoEditor", () => { const makeEditor = () => @@ -91,19 +92,22 @@ describe("gitmostInsertTranscriptIntoEditor", () => { editor.destroy(); }); - it("neutralizes col-0 markdown block triggers with a leading ZWSP (git-sync safety)", () => { + it("inserts col-0 markdown block triggers as verbatim paragraph text (no ZWSP workaround)", () => { const editor = makeEditor(); - // Trigger lines (some with a leaked indent) + a normal prefixed line. + // Trigger lines (some with a leaked indent) + a normal prefixed line. The + // git-sync serializer now block-escapes a leading trigger itself, so the + // bridge inserts each line's TEXT byte-exact (only the leaked indent is + // trimmed) — no invisible ZWSP is prepended anymore. const inserted = gitmostInsertTranscriptIntoEditor( editor, [ "- dash", - " > quote", // leading indent must be trimmed then neutralized + " > quote", // leading indent is trimmed, text otherwise verbatim "# hash", "1. one", "> [!info] note", "```js", - "---", // solid thematic break -> horizontalRule (text-losing) if unneutralized + "---", "***", "___", "You: normal line", @@ -116,20 +120,23 @@ describe("gitmostInsertTranscriptIntoEditor", () => { .map((n: any) => n.content?.[0]?.text) .filter((t: any) => typeof t === "string") as string[]; - // Every block-trigger line is prefixed with the invisible ZWSP (indent - // trimmed first); the normal `You:` line is left byte-exact. + // Each trigger line is stored as its own byte-exact text (indent trimmed); + // the git-sync round-trip keeps it a paragraph via the serializer's + // block-escape, so no ZWSP is needed here. expect(texts).toEqual([ - ZWSP + "- dash", - ZWSP + "> quote", - ZWSP + "# hash", - ZWSP + "1. one", - ZWSP + "> [!info] note", - ZWSP + "```js", - ZWSP + "---", - ZWSP + "***", - ZWSP + "___", + "- dash", + "> quote", + "# hash", + "1. one", + "> [!info] note", + "```js", + "---", + "***", + "___", "You: normal line", ]); + // Guard: no invisible ZWSP leaked into any inserted line. + for (const t of texts) expect(t).not.toContain(ZWSP); editor.destroy(); }); diff --git a/apps/client/src/features/editor/gitmost/gitmost-recording.ts b/apps/client/src/features/editor/gitmost/gitmost-recording.ts index 2856ac08..1be0d638 100644 --- a/apps/client/src/features/editor/gitmost/gitmost-recording.ts +++ b/apps/client/src/features/editor/gitmost/gitmost-recording.ts @@ -240,45 +240,22 @@ export async function gitmostUploadFileToEditor( } } -// Zero-width space (U+200B). Prepended to a transcript line that begins with a -// markdown BLOCK trigger: it is invisible in the rendered doc but shifts the -// trigger off column 0, so the git-sync doc->markdown->doc round-trip keeps the -// line a plain paragraph (see GITMOST_MD_BLOCK_TRIGGER_RE). -const GITMOST_ZWSP = "​"; - -// A markdown BLOCK-level construct that, sitting at column 0 of a paragraph -// line, the git-sync markdown serializer (packages/prosemirror-markdown -// markdown-converter.ts, `case "paragraph"`) would re-parse into a NON-paragraph -// block on the doc->markdown->doc cycle. That serializer emits paragraph text -// verbatim with NO block-escape (the pre-existing root cause), so a leading -// `#`/`-`/`*`/`+`/`>`, an ordered-list `N.`/`N)`, a code fence ```/~~~, a table -// `|`, or a `> [!info]` callout opener would silently become a heading / list / -// quote / code block / table / callout. The final alternative matches a WHOLE- -// LINE thematic break — solid `---`/`***`/`___` or spaced `- - -`/`_ _ _` (3+ of -// the same `-`/`*`/`_`) — which round-trips into a `horizontalRule`; because -// that node carries NO text, an un-neutralized separator line would LOSE its -// text entirely (worse than the list/quote case). This matches a TRIMMED line's -// start; the transcript's own `You:` / `Speaker N:` prefix begins with a letter -// and never matches, so prefixed lines are left byte-exact. -const GITMOST_MD_BLOCK_TRIGGER_RE = - /^(?:#{1,6}(?:\s|$)|[-*+](?:\s|$)|>|\d+[.)](?:\s|$)|```|~~~|\||([-*_])(?:\s*\1){2,}\s*$)/; - // Append a transcript block BELOW the recording's audio node in a live editor: // a "Transcript" heading followed by one paragraph per non-empty transcript // line. The transcript is plain text, `\n`-separated, each line already // formatted as `You: ...` / `Speaker N: ...` by the native host — line text is // inserted as a TEXT node (never HTML/markdown), so there is no injection or // mark-parsing surface. Each kept line is trimmed (drops an indent that would -// both leak into the display and, at col 0, form a markdown block trigger) and, -// if it still begins with a col-0 markdown block trigger, gets an invisible -// zero-width space prepended so the git-sync round-trip cannot turn it into a -// list/quote/heading/callout/code/table (defensive boundary against the -// serializer's missing block-escape). This is best-effort and meant to run -// AFTER the audio has already been inserted; the caller must guard against a -// throw so a transcript failure never fails the (already successful) recording. -// Returns true when a block was inserted, false when there was nothing to -// insert (transcript undefined/empty/not-a-string). A non-string value is a -// no-op, not an error. +// leak into the display). A line that begins with a col-0 markdown block +// trigger (`#`/`-`/`>`/`1.`/fence/`---`/…) needs no client-side workaround: the +// git-sync serializer (packages/prosemirror-markdown, `case "paragraph"`) now +// block-escapes such a leading trigger, so the doc->markdown->doc round-trip +// keeps the line a paragraph on its own — the former invisible-ZWSP defense is +// gone. This is best-effort and meant to run AFTER the audio has already been +// inserted; the caller must guard against a throw so a transcript failure never +// fails the (already successful) recording. Returns true when a block was +// inserted, false when there was nothing to insert (transcript +// undefined/empty/not-a-string). A non-string value is a no-op, not an error. export function gitmostInsertTranscriptIntoEditor( editor: Editor, transcript: unknown, @@ -288,13 +265,7 @@ export function gitmostInsertTranscriptIntoEditor( .split("\n") // Trim each line and drop blank (whitespace-only) ones. .map((line) => line.trim()) - .filter((line) => line.length > 0) - // Neutralize a col-0 markdown block trigger with an invisible ZWSP so the - // git-sync round-trip keeps the line a paragraph. Host lines (`You:` / - // `Speaker N:`) never match and stay byte-exact. - .map((line) => - GITMOST_MD_BLOCK_TRIGGER_RE.test(line) ? GITMOST_ZWSP + line : line, - ); + .filter((line) => line.length > 0); if (lines.length === 0) return false; const content = [ diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 0b4d8d80..421319a4 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -53,8 +53,10 @@ import { extractPageSlugId, } from '../../../integrations/export/utils'; import { canonicalizeFootnotes } from '@docmost/editor-ext'; -import { markdownToProseMirror } from '@docmost/prosemirror-markdown'; -import { normalizeForeignMarkdown } from '../../../integrations/import/utils/foreign-markdown'; +import { + markdownToProseMirror, + normalizeForeignMarkdown, +} from '@docmost/prosemirror-markdown'; import { WatcherService } from '../../watcher/watcher.service'; import { sql } from 'kysely'; import { TransclusionService } from '../transclusion/transclusion.service'; diff --git a/apps/server/src/integrations/import/services/file-import-task.service.ts b/apps/server/src/integrations/import/services/file-import-task.service.ts index a5115c43..ab1debab 100644 --- a/apps/server/src/integrations/import/services/file-import-task.service.ts +++ b/apps/server/src/integrations/import/services/file-import-task.service.ts @@ -22,10 +22,12 @@ import { v7 } from 'uuid'; import { generateJitteredKeyBetween } from 'fractional-indexing-jittered'; import { FileTask, InsertablePage } from '@docmost/db/types/entity.types'; import { canonicalizeFootnotes } from '@docmost/editor-ext'; -import { markdownToProseMirror } from '@docmost/prosemirror-markdown'; +import { + markdownToProseMirror, + normalizeForeignMarkdown, +} from '@docmost/prosemirror-markdown'; import { getProsemirrorContent } from '../../../common/helpers/prosemirror/utils'; import { formatImportHtml } from '../utils/import-formatter'; -import { normalizeForeignMarkdown } from '../utils/foreign-markdown'; import { buildAttachmentCandidates, collectMarkdownAndHtmlFiles, diff --git a/apps/server/src/integrations/import/services/import.service.ts b/apps/server/src/integrations/import/services/import.service.ts index dd86d71e..7c480ef2 100644 --- a/apps/server/src/integrations/import/services/import.service.ts +++ b/apps/server/src/integrations/import/services/import.service.ts @@ -18,8 +18,10 @@ import { generateJitteredKeyBetween } from 'fractional-indexing-jittered'; import { TiptapTransformer } from '@hocuspocus/transformer'; import * as Y from 'yjs'; import { canonicalizeFootnotes } from '@docmost/editor-ext'; -import { markdownToProseMirror } from '@docmost/prosemirror-markdown'; -import { normalizeForeignMarkdown } from '../utils/foreign-markdown'; +import { + markdownToProseMirror, + normalizeForeignMarkdown, +} from '@docmost/prosemirror-markdown'; import { FileTaskStatus, FileTaskType, diff --git a/packages/git-sync/src/engine/stabilize.ts b/packages/git-sync/src/engine/stabilize.ts index ce1acdcf..e6769cd0 100644 --- a/packages/git-sync/src/engine/stabilize.ts +++ b/packages/git-sync/src/engine/stabilize.ts @@ -72,7 +72,13 @@ export async function stabilizePageFile( * keeps re-pulls of an unchanged page byte-identical (no churn, loop-guard). */ export async function stabilizePageBody(content: unknown): Promise { - const md1 = convertProseMirrorToMarkdown(content); + // git-sync is the LOSSLESS mirror path, so run the serializer in `strict` + // mode: a node/mark type the converter has no case for (e.g. one added to the + // schema without a matching serializer arm) throws a ConverterLossError here + // rather than silently degrading — surfacing the loss loudly at write time + // instead of committing a lossy file. Valid content (every current schema type + // has a case) is unaffected. + const md1 = convertProseMirrorToMarkdown(content, { strict: true }); const doc2 = await markdownToProseMirror(md1); - return convertProseMirrorToMarkdown(doc2); + return convertProseMirrorToMarkdown(doc2, { strict: true }); } diff --git a/packages/git-sync/test/stabilize.test.ts b/packages/git-sync/test/stabilize.test.ts index c781546e..66191aa0 100644 --- a/packages/git-sync/test/stabilize.test.ts +++ b/packages/git-sync/test/stabilize.test.ts @@ -4,6 +4,7 @@ import { stabilizePageFile, type PageMeta } from '../src/engine/stabilize.js'; // global DOM via jsdom at module load time (required for @tiptap/html under Node). import { markdownToProseMirror } from '@docmost/prosemirror-markdown'; import { parseDocmostMarkdown } from '@docmost/prosemirror-markdown'; +import { ConverterLossError } from '@docmost/prosemirror-markdown'; // stabilize.ts (SPEC §11 normalize-on-write) was 0% covered (only the gated e2e // touched it). stabilizePageFile is import-testable: build a small ProseMirror @@ -66,6 +67,23 @@ describe('stabilizePageFile — normalize-on-write fixpoint (SPEC §11)', () => expect(body1).toContain('data-src="/d.drawio"'); }); + it('runs the serializer in STRICT mode — an unmappable node throws, not a lossy write (#493)', async () => { + // git-sync is the lossless mirror path: a node type the converter has no + // case for (here a fabricated one, standing in for a schema type added + // without a matching serializer arm) must surface loudly at write time + // rather than being silently flattened into a lossy .md file. + const content = { + type: 'doc', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: 'ok' }] }, + { type: 'quantumWidget', content: [{ type: 'text', text: 'lost?' }] }, + ], + }; + await expect(stabilizePageFile(content, meta)).rejects.toBeInstanceOf( + ConverterLossError, + ); + }); + it('already-stable content is unchanged by the pass (idempotent)', async () => { // Plain prose is already a fixpoint; stabilizing it once and twice agree. const content = { diff --git a/packages/mcp/src/lib/collaboration.ts b/packages/mcp/src/lib/collaboration.ts index 08764e77..0de4e497 100644 --- a/packages/mcp/src/lib/collaboration.ts +++ b/packages/mcp/src/lib/collaboration.ts @@ -10,7 +10,10 @@ import { JSDOM } from "jsdom"; // handled there). MCP consumes it directly instead of maintaining its own // drifted marked pipeline; only the collab/yjs write glue and the footnote // canonicalization wrapper stay mcp-side. -import { markdownToProseMirror } from "@docmost/prosemirror-markdown"; +import { + markdownToProseMirror, + normalizeAgentMarkdown, +} from "@docmost/prosemirror-markdown"; import { docmostExtensions, docmostSchema } from "./docmost-schema.js"; import { withPageLock } from "./page-lock.js"; import type { PageId } from "./page-id.js"; @@ -21,6 +24,7 @@ import { } from "@docmost/prosemirror-markdown"; import { canonicalizeFootnotes } from "./footnote-canonicalize.js"; import { normalizeAndMergeFootnotes } from "./footnote-normalize-merge.js"; +import { regraftResolvedComments } from "./comment-anchor.js"; import { VerifyReport } from "./diff.js"; import { acquireCollabSession } from "./collab-session.js"; @@ -98,6 +102,15 @@ global.WebSocket = WebSocket; * plain `markdownToProseMirror` (no canonicalization) — safe now because inline * `^[body]` footnotes carry their body at the reference point, so a comment can * no longer produce a reference-less footnote definition to be dropped. + * + * #493: `normalizeAgentMarkdown` runs FIRST, so an agent's `updatePageMarkdown` + * body gets the SAME GFM `[^id]` reference-footnote -> inline `^[body]` rewrite as + * the server import path (instead of the reference leaking as literal text / a + * bogus link). It DELIBERATELY does NOT strip a leading YAML front-matter block: + * a full-body agent rewrite that opens with a `---…---` is (almost) always a + * horizontalRule the serializer emitted, and stripping it would silently drop the + * page's leading content (#493 review). The front-matter strip stays on the + * server FILE-import boundary only (`normalizeForeignMarkdown`). */ export async function markdownToProseMirrorCanonical( markdownContent: string, @@ -106,7 +119,9 @@ export async function markdownToProseMirrorCanonical( // canonicalizing, so the canonicalizer re-hangs references and drops the // now-orphaned duplicate definitions. return canonicalizeFootnotes( - normalizeAndMergeFootnotes(await markdownToProseMirror(markdownContent)), + normalizeAndMergeFootnotes( + await markdownToProseMirror(normalizeAgentMarkdown(markdownContent)), + ), ); } @@ -348,6 +363,12 @@ export async function updatePageContentRealtime( pageId, collabToken, baseUrl, - () => tiptapJson, + // #493: an agent read HIDES resolved-comment anchors (#337), so the markdown + // it sends here no longer carries them — a naive full rewrite would erase + // every resolved comment mark. Re-graft the resolved marks from the LIVE doc + // onto the matching text in the freshly-imported body. Active comments are + // untouched (they ride through the markdown themselves); a resolved span whose + // text the agent changed simply does not re-anchor and is dropped. + (liveDoc) => regraftResolvedComments(liveDoc, tiptapJson), ); } diff --git a/packages/mcp/src/lib/comment-anchor.ts b/packages/mcp/src/lib/comment-anchor.ts index accf6c16..0e6f6ef8 100644 --- a/packages/mcp/src/lib/comment-anchor.ts +++ b/packages/mcp/src/lib/comment-anchor.ts @@ -312,10 +312,9 @@ export function canAnchorInDoc(doc: any, selection: string): boolean { function spliceCommentMark( blockContent: any[], match: AnchorMatch, - commentId: string, + commentMark: any, ): void { const { startChild, startOffset, endChild, endOffset } = match; - const commentMark = makeCommentMark(commentId); const fragments: any[] = []; for (let k = startChild; k <= endChild; k++) { @@ -452,6 +451,22 @@ export function applyAnchorInDoc( doc: any, selection: string, commentId: string, +): boolean { + return applyCommentMarkInDoc(doc, selection, makeCommentMark(commentId)); +} + +/** + * Core of {@link applyAnchorInDoc}, but splices an ARBITRARY comment mark object + * (not just a fresh `{ commentId, resolved:false }`) across the first matching + * range. This lets a caller re-apply a mark that carries `resolved:true` and any + * other stored attrs. Depth-first (same order as canAnchorInDoc); mutates in + * place on the first matching block and returns true, else returns false without + * mutating. + */ +export function applyCommentMarkInDoc( + doc: any, + selection: string, + commentMark: any, ): boolean { const { selection: effective, found } = resolveAnchorSelection(doc, selection); if (!found) return false; @@ -460,7 +475,7 @@ export function applyAnchorInDoc( if (!Array.isArray(node.content)) return false; const match = findAnchorInBlock(node.content, effective); if (match) { - spliceCommentMark(node.content, match, commentId); + spliceCommentMark(node.content, match, commentMark); return true; } for (const child of node.content) { @@ -472,3 +487,97 @@ export function applyAnchorInDoc( }; return visit(doc, 0); } + +/** A resolved inline-comment span lifted from a doc: its mark + anchored text. */ +export interface ResolvedCommentSpan { + commentId: string; + /** The full comment mark (carrying `resolved:true` + any stored attrs). */ + mark: any; + /** The concatenated raw text the mark spans — used as the re-anchor selection. */ + text: string; +} + +/** True when a text node carries a RESOLVED comment mark; returns that mark. */ +function resolvedCommentMarkOf(node: any): any | null { + if (!node || node.type !== "text" || !Array.isArray(node.marks)) return null; + return ( + node.marks.find( + (m: any) => + m && m.type === "comment" && m.attrs?.resolved === true && m.attrs?.commentId, + ) || null + ); +} + +/** + * Collect every RESOLVED inline-comment span in `doc`, in document order. Within + * each block's direct content, a maximal run of consecutive text nodes sharing + * the same resolved `commentId` is ONE span; its concatenated raw text is the + * selection used to re-anchor it elsewhere. Active (unresolved) comment marks are + * ignored — they survive a markdown round-trip on their own (a page read emits + * their `` wrapper), whereas resolved anchors are hidden + * from agent reads (#337) and would be erased by a full-body markdown rewrite. + */ +export function collectResolvedCommentSpans(doc: any): ResolvedCommentSpan[] { + const spans: ResolvedCommentSpan[] = []; + const visit = (node: any, depth: number): void => { + if (depth > MAX_DEPTH || !node || typeof node !== "object") return; + if (!Array.isArray(node.content)) return; + const content = node.content; + let i = 0; + while (i < content.length) { + const mark = resolvedCommentMarkOf(content[i]); + if (mark) { + const commentId = mark.attrs.commentId; + let text = ""; + let j = i; + while (j < content.length) { + const mj = resolvedCommentMarkOf(content[j]); + if (!mj || mj.attrs.commentId !== commentId) break; + text += typeof content[j].text === "string" ? content[j].text : ""; + j++; + } + if (text.length > 0) spans.push({ commentId, mark, text }); + i = j > i ? j : i + 1; + } else { + i++; + } + } + for (const child of content) { + if (child && typeof child === "object" && Array.isArray(child.content)) { + visit(child, depth + 1); + } + } + }; + visit(doc, 0); + return spans; +} + +/** + * Re-graft RESOLVED comment marks from `oldDoc` onto matching text ranges in + * `newDoc`, returning a NEW doc (never mutates the inputs). + * + * WHY (#493): an agent read hides resolved-comment anchors (#337), so the + * markdown it sends to a FULL-body rewrite (`updatePageMarkdown`) no longer + * carries them — a naive full write would erase every resolved comment mark. + * This restores them: each resolved span from the previous document is re-anchored + * onto the SAME text in the newly-imported body (first occurrence, using the + * shared anchoring / markdown-strip fallback), preserving `resolved:true` and the + * stored attrs. A span whose text the agent changed or deleted simply does not + * re-anchor and is dropped (its anchor is gone; it was already resolved). Active + * comments are untouched — they ride through the markdown themselves. + */ +export function regraftResolvedComments(oldDoc: any, newDoc: T): T { + if (!newDoc || typeof newDoc !== "object") return newDoc; + const spans = collectResolvedCommentSpans(oldDoc); + if (spans.length === 0) return newDoc; + const out = + typeof structuredClone === "function" + ? structuredClone(newDoc) + : (JSON.parse(JSON.stringify(newDoc)) as T); + for (const span of spans) { + // Clone the mark so the new document never shares a mark object with oldDoc. + const markClone = { type: "comment", attrs: { ...span.mark.attrs } }; + applyCommentMarkInDoc(out, span.text, markClone); + } + return out; +} diff --git a/packages/mcp/src/lib/text-normalize.ts b/packages/mcp/src/lib/text-normalize.ts index e80bcfdb..f8480417 100644 --- a/packages/mcp/src/lib/text-normalize.ts +++ b/packages/mcp/src/lib/text-normalize.ts @@ -1,64 +1,30 @@ /** - * Locator normalization: strip inline markdown wrappers and trailing - * decoration from a LOCATOR string so a find/anchor that the model wrote with - * markdown (or a stray emoji) can still match the document's plain text. + * Locator normalization helpers for mcp. The two PRIMITIVES — + * `stripInlineMarkdown` (lenient locator normalizer) and `stripWrappersAndLinks` + * (strict balanced-wrapper/link collapse) — live in the canonical package + * `@docmost/prosemirror-markdown` (#493 dedup: they used to be forked verbatim + * here). This module now only re-exports `stripInlineMarkdown` and adds the two + * mcp-only helpers built on top: `stripBalancedWrappers` and `closestBlockHint`. * - * This is used ONLY as a fallback for LOCATING (after an exact match fails); - * it is never applied to replacement text or inserted node content, so no - * formatting is ever lost. + * They are used ONLY as a fallback for LOCATING (after an exact match fails) and + * for formatting-vs-plain intent detection; never applied to replacement text or + * inserted node content, so no formatting is ever lost. */ +import { + stripInlineMarkdown, + stripWrappersAndLinks, +} from "@docmost/prosemirror-markdown"; -/** Maximum unwrap passes, so pathological/nested input cannot loop forever. */ -const MAX_PASSES = 8; +// Re-export the canonical locator normalizer so mcp call sites keep importing it +// from `./text-normalize.js` unchanged. +export { stripInlineMarkdown }; /** - * Inline emphasis/code/strikethrough wrappers, strong BEFORE emphasis so - * `**x**` collapses to `x` rather than leaving a stray `*x*`. Each pattern is - * non-greedy and capture group 1 is the inner text. Applied repeatedly until - * the string stops changing (nested wrappers like `**_x_**`). - */ -const WRAPPER_PATTERNS: RegExp[] = [ - /\*\*([^*]+?)\*\*/g, // **x** - /__([^_]+?)__/g, // __x__ - /~~([^~]+?)~~/g, // ~~x~~ - /\*([^*]+?)\*/g, // *x* - /_([^_]+?)_/g, // _x_ - /``([^`]+?)``/g, // ``x`` - /`([^`]+?)`/g, // `x` -]; - -/** Links/images -> their visible text. `!?` covers both `[t](u)` and `![a](s)`. */ -const LINK_IMAGE_RE = /!?\[([^\]]*)\]\([^)]*\)/g; - -/** - * Apply ONLY the two balanced/link passes shared by both normalizers: first - * collapse links/images to their visible text, then collapse balanced inline - * wrappers repeatedly until stable. Does NOT trim decoration, does NOT guard - * against an empty result — it returns exactly the transformed string. - */ -function stripWrappersAndLinks(s: string): string { - // 1. Links/images -> their visible text. - let out = s.replace(LINK_IMAGE_RE, "$1"); - - // 2. Strip balanced wrappers, repeating until the string is stable so nested - // wrappers (`**_x_**`) and adjacent runs both collapse. - for (let pass = 0; pass < MAX_PASSES; pass++) { - const before = out; - for (const re of WRAPPER_PATTERNS) { - out = out.replace(re, "$1"); - } - if (out === before) break; - } - return out; -} - -/** - * STRICT formatting detector — distinct from the lenient locator - * normalization below. It strips ONLY what unambiguously is markdown markup: - * 1. links/images `[text](url)` -> `text`, `![alt](src)` -> `alt`, and - * 2. balanced inline `**`/`__`/`~~`/`*`/`_`/`` ` `` wrappers (repeat-until-stable), - * and DELIBERATELY does NOT trim leading/trailing whitespace, emoji, or lone - * marker chars (the lenient extras `stripInlineMarkdown` does in its step 3). + * STRICT formatting detector — distinct from the lenient locator normalization. + * It strips ONLY what unambiguously is markdown markup (links/images to visible + * text, and balanced inline `**`/`__`/`~~`/`*`/`_`/`` ` `` wrappers) and + * DELIBERATELY does NOT trim leading/trailing whitespace, emoji, or lone marker + * chars (the lenient extras `stripInlineMarkdown` does). * * It exists ONLY to recognize formatting-vs-plain INTENT in `applyTextEdits` * (deciding whether find/replace differ purely by markdown markers). Because it @@ -77,44 +43,6 @@ export function stripBalancedWrappers(s: string): string { return stripWrappersAndLinks(s); } -/** - * Conservatively strip inline markdown from a locator string. - * - * Deterministic, order-fixed steps: - * 1. Links/images: `[text](url)` -> `text`, `![alt](src)` -> `alt`. - * 2. Balanced inline wrappers (strong before emphasis, code, strikethrough), - * applied repeatedly until stable for nested cases. - * 3. Trim leading/trailing decoration only: whitespace, leftover marker chars - * (`* _ ~ \``) and emoji. Letters/digits and sentence punctuation (`.`/`,` - * etc.) are NEVER trimmed. - * - * If the result is empty (e.g. the input was only markers like `***`), the - * ORIGINAL string is returned so a locator can never normalize down to "" and - * match everything. - */ -export function stripInlineMarkdown(s: string): string { - if (typeof s !== "string" || s.length === 0) return s; - - // 1 + 2. Shared link/image and balanced-wrapper passes. - let out = stripWrappersAndLinks(s); - - // 3. Trim leading/trailing decoration: whitespace, leftover markdown markers, - // and emoji (Extended_Pictographic plus the VS16 / ZWJ joiners, plus the - // regional-indicator range U+1F1E6–U+1F1FF for flag emoji, which are NOT - // Extended_Pictographic). The `u` flag enables the Unicode property escape. - // Anchored runs only — interior text and sentence punctuation are untouched. - const DECORATION = - "[\\s*_~\\x60\\p{Extended_Pictographic}\\u{1F1E6}-\\u{1F1FF}\\u{FE0F}\\u{200D}]+"; - out = out - .replace(new RegExp("^" + DECORATION, "u"), "") - .replace(new RegExp(DECORATION + "$", "u"), ""); - - // 4. Never normalize a locator down to nothing. - if (out.length === 0) return s; - - return out; -} - /** * Build a bounded "closest text" hint for an anchor/find MISS, shared by * editPageText (json-edit) and createComment (client) so both surface the diff --git a/packages/mcp/test/unit/regraft-resolved-comments.test.mjs b/packages/mcp/test/unit/regraft-resolved-comments.test.mjs new file mode 100644 index 00000000..fa3c2fa8 --- /dev/null +++ b/packages/mcp/test/unit/regraft-resolved-comments.test.mjs @@ -0,0 +1,107 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + collectResolvedCommentSpans, + regraftResolvedComments, + applyCommentMarkInDoc, +} from "../../build/lib/comment-anchor.js"; + +/** + * #493 commit 6 — resolved-comment anchors must survive a full markdown rewrite + * (updatePageMarkdown). An agent read HIDES resolved anchors (#337), so its + * markdown drops them; a naive full write would erase the resolved comment marks. + * `regraftResolvedComments(oldDoc, newDoc)` re-anchors them onto the matching + * text. These exercise the real anchoring (no mock). + */ + +const doc = (...content) => ({ type: "doc", content }); +const para = (...content) => ({ type: "paragraph", content }); +const text = (t, marks) => (marks ? { type: "text", text: t, marks } : { type: "text", text: t }); +const resolvedComment = (commentId) => ({ type: "comment", attrs: { commentId, resolved: true } }); +const activeComment = (commentId) => ({ type: "comment", attrs: { commentId, resolved: false } }); + +/** The comment mark on a text node, or null. */ +function commentMarkOf(node) { + const marks = Array.isArray(node?.marks) ? node.marks : []; + return marks.find((m) => m && m.type === "comment") || null; +} +/** Flatten every text node in a doc (deep). */ +function textNodes(node, out = []) { + if (!node || typeof node !== "object") return out; + if (node.type === "text") out.push(node); + if (Array.isArray(node.content)) for (const c of node.content) textNodes(c, out); + return out; +} + +test("collectResolvedCommentSpans: only resolved marks, concatenated across a run", () => { + const old = doc( + para( + text("keep "), + text("resolved bit", [resolvedComment("r1")]), + text(" and "), + text("active bit", [activeComment("a1")]), + ), + ); + const spans = collectResolvedCommentSpans(old); + assert.equal(spans.length, 1); + assert.equal(spans[0].commentId, "r1"); + assert.equal(spans[0].text, "resolved bit"); + assert.equal(spans[0].mark.attrs.resolved, true); +}); + +test("regraft restores a resolved mark the agent's markdown dropped", () => { + // OLD doc has a resolved comment on "important note". + const old = doc(para(text("An "), text("important note", [resolvedComment("r1")]), text(" here."))); + // NEW doc (re-imported from the agent's markdown) has the SAME text but NO + // comment mark — the resolved anchor was hidden on read. + const fresh = doc(para(text("An important note here."))); + + const out = regraftResolvedComments(old, fresh); + // Inputs are not mutated. + assert.equal(commentMarkOf(textNodes(fresh)[0]), null); + // The resolved mark is back on exactly "important note". + const marked = textNodes(out).filter((n) => commentMarkOf(n)); + assert.equal(marked.length, 1); + assert.equal(marked[0].text, "important note"); + assert.equal(commentMarkOf(marked[0]).attrs.commentId, "r1"); + assert.equal(commentMarkOf(marked[0]).attrs.resolved, true); +}); + +test("a resolved span whose text the agent changed is dropped (no re-anchor)", () => { + const old = doc(para(text("stale text", [resolvedComment("r1")]))); + const fresh = doc(para(text("completely rewritten body"))); + const out = regraftResolvedComments(old, fresh); + assert.equal(textNodes(out).filter((n) => commentMarkOf(n)).length, 0); +}); + +test("regraft is a no-op when the old doc has no resolved comments", () => { + const old = doc(para(text("plain "), text("active", [activeComment("a1")]))); + const fresh = doc(para(text("plain active"))); + const out = regraftResolvedComments(old, fresh); + assert.equal(textNodes(out).filter((n) => commentMarkOf(n)).length, 0); +}); + +test("multiple distinct resolved comments are all restored", () => { + const old = doc( + para(text("first", [resolvedComment("r1")]), text(" middle "), text("second", [resolvedComment("r2")])), + ); + const fresh = doc(para(text("first middle second"))); + const out = regraftResolvedComments(old, fresh); + const byId = Object.fromEntries( + textNodes(out) + .filter((n) => commentMarkOf(n)) + .map((n) => [commentMarkOf(n).attrs.commentId, n.text]), + ); + assert.equal(byId["r1"], "first"); + assert.equal(byId["r2"], "second"); +}); + +test("applyCommentMarkInDoc preserves an arbitrary mark's attrs (resolved:true)", () => { + const d = doc(para(text("anchor me somewhere"))); + const ok = applyCommentMarkInDoc(d, "anchor me", { type: "comment", attrs: { commentId: "x9", resolved: true } }); + assert.equal(ok, true); + const marked = textNodes(d).filter((n) => commentMarkOf(n)); + assert.equal(marked[0].text, "anchor me"); + assert.equal(commentMarkOf(marked[0]).attrs.resolved, true); +}); diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.ts b/packages/prosemirror-markdown/src/lib/foreign-markdown.ts similarity index 81% rename from apps/server/src/integrations/import/utils/foreign-markdown.ts rename to packages/prosemirror-markdown/src/lib/foreign-markdown.ts index dd7b012a..a49bc4e3 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.ts +++ b/packages/prosemirror-markdown/src/lib/foreign-markdown.ts @@ -1,7 +1,14 @@ /** * Foreign-markdown normalizer — an input-liberal / output-canonical adapter that * runs at the IMPORT boundary, BEFORE the canonical parser - * (`markdownToProseMirror` from `@docmost/prosemirror-markdown`). + * (`markdownToProseMirror`, this package). + * + * OWNED BY THIS PACKAGE (#493): the normalizer used to live only in + * apps/server's import path, so the MCP page-write path (`updatePageMarkdown` -> + * `markdownToProseMirrorCanonical`) handled the SAME foreign input differently + * (no front-matter strip, no `[^id]` reference-footnote rewrite) than the server + * importer. Moving it here — and calling it from `markdownToProseMirrorCanonical` + * — makes every canonical import boundary treat foreign markdown identically. * * The canonical parser is deliberately STRICT: it only understands Docmost's * canonical markdown surface (Obsidian-style `> [!type]` callouts, Pandoc/Obsidian @@ -247,11 +254,18 @@ function convertReferenceFootnotes(markdown: string): string { const YAML_FRONT_MATTER_RE = /^\uFEFF?---\n[\s\S]*?\n---\n?/; /** - * Normalize a foreign markdown string into Docmost's canonical markdown surface - * so the strict canonical parser accepts it losslessly: normalize line endings, - * strip a leading YAML front-matter block, then rewrite GFM reference footnotes - * into inline footnotes. Add further fixture-driven foreign-surface cases here as - * they are found. + * Normalize a foreign markdown string from a FILE IMPORT into Docmost's canonical + * markdown surface so the strict canonical parser accepts it losslessly: normalize + * line endings, strip a leading YAML front-matter block, then rewrite GFM reference + * footnotes into inline footnotes. Add further fixture-driven foreign-surface cases + * here as they are found. + * + * FRONT-MATTER STRIP IS IMPORT-ONLY (#493 review): use this ONLY at the server + * file-import boundary, where a `.md` file really can open with an Obsidian/Hugo + * YAML header. Do NOT use it on the canonical AGENT-WRITE path — see + * {@link normalizeAgentMarkdown} for why a full-body agent rewrite must NOT strip + * a leading `---…---` (it is normally a horizontalRule the serializer emitted, and + * stripping it would silently drop the page's leading content). */ export function normalizeForeignMarkdown(markdown: string): string { if (!markdown) return markdown; @@ -264,3 +278,26 @@ export function normalizeForeignMarkdown(markdown: string): string { const withoutFrontMatter = src.replace(YAML_FRONT_MATTER_RE, '').trimStart(); return convertReferenceFootnotes(withoutFrontMatter); } + +/** + * Canonical AGENT-WRITE normalization: normalize line endings and rewrite GFM + * `[^id]` reference footnotes to inline `^[body]` — but DELIBERATELY NOT strip a + * leading YAML front-matter block. + * + * WHY the split (#493 review): the reference-footnote rewrite is the drift the + * MCP page-write path (`updatePageMarkdown` -> `markdownToProseMirrorCanonical`) + * needed unified with the server import (an agent may paste GFM footnotes). The + * front-matter strip, however, is a FILE-import concern: on a full-body agent + * rewrite a leading `---…---` is (almost) always a `horizontalRule` the + * serializer emitted plus a later rule/heading — NOT a foreign YAML header — so + * `YAML_FRONT_MATTER_RE` would match it and SILENTLY DELETE the page's leading + * content (a page that starts with a horizontal rule and contains a second `---` + * lost everything up to it). Agent writes must never lose already-stored content, + * so this variant skips the strip. It IS a no-op on canonical serialized content + * (which never emits `[^id]:` reference-definition lines). + */ +export function normalizeAgentMarkdown(markdown: string): string { + if (!markdown) return markdown; + const src = markdown.replace(/\r\n/g, '\n'); + return convertReferenceFootnotes(src); +} diff --git a/packages/prosemirror-markdown/src/lib/index.ts b/packages/prosemirror-markdown/src/lib/index.ts index cbf8f462..8bdfeb14 100644 --- a/packages/prosemirror-markdown/src/lib/index.ts +++ b/packages/prosemirror-markdown/src/lib/index.ts @@ -15,7 +15,10 @@ export { } from "./markdown-document.js"; export type { DocmostMdMeta } from "./markdown-document.js"; -export { convertProseMirrorToMarkdown } from "./markdown-converter.js"; +export { + convertProseMirrorToMarkdown, + ConverterLossError, +} from "./markdown-converter.js"; export type { ConvertProseMirrorToMarkdownOptions } from "./markdown-converter.js"; export { @@ -23,6 +26,19 @@ export { markdownToProseMirrorSync, } from "./markdown-to-prosemirror.js"; +// Foreign-markdown normalizer (#493): the input-liberal pre-pass that rewrites +// GFM `[^id]` reference footnotes to canonical inline `^[body]`. Two variants: +// `normalizeForeignMarkdown` (server FILE-import boundary) ALSO strips a leading +// YAML front-matter block; `normalizeAgentMarkdown` (canonical AGENT-WRITE path, +// mcp `markdownToProseMirrorCanonical`) does NOT — a full-body agent rewrite must +// not lose a leading `---…---` horizontalRule to the front-matter strip (#493 +// review). The reference-footnote rewrite is shared so agent + import stay unified +// where it matters, without the content-losing strip on the write path. +export { + normalizeForeignMarkdown, + normalizeAgentMarkdown, +} from "./foreign-markdown.js"; + // The Docmost tiptap schema mirror. Exposed so consumers (and the sync // engine's schema-validity regression tests) can build the exact ProseMirror // schema the converter targets. @@ -76,6 +92,17 @@ export type { OutlineEntry } from "./node-ops.js"; // string (#414: single copy shared by mcp and the CommonJS server app). export { parseNodeArg } from "./parse-node-arg.js"; +// Locator markdown-stripping (#493 dedup): the single canonical copy of the +// markdown-tolerant anchor-normalization primitives, imported by mcp's +// text-normalize.ts instead of a forked duplicate. `stripInlineMarkdown` is the +// lenient locator normalizer (trims stray decoration); `stripWrappersAndLinks` +// is the strict balanced-wrapper/link primitive mcp builds `stripBalancedWrappers` +// on top of. +export { + stripInlineMarkdown, + stripWrappersAndLinks, +} from "./text-normalize.js"; + // Inline-footnote authoring convention (#414: single copy, formerly the mcp // `footnote-authoring.ts` fork), shared with the importer's `assembleFootnotes`. export { diff --git a/packages/prosemirror-markdown/src/lib/markdown-converter.ts b/packages/prosemirror-markdown/src/lib/markdown-converter.ts index 8389d2b0..3f647a75 100644 --- a/packages/prosemirror-markdown/src/lib/markdown-converter.ts +++ b/packages/prosemirror-markdown/src/lib/markdown-converter.ts @@ -33,6 +33,26 @@ import { */ const MAX_NODE_DEPTH = 400; +/** + * Thrown by {@link convertProseMirrorToMarkdown} in `strict` mode when it hits a + * node or mark type it has no lossless markdown form for (the serializer would + * otherwise silently degrade it — drop an unknown mark, flatten an unknown node + * to its children). Carries the offending kind/name so a caller (git-sync) can + * surface exactly what would have been lost. + */ +export class ConverterLossError extends Error { + readonly kind: "node" | "mark"; + readonly typeName: string; + constructor(kind: "node" | "mark", typeName: string) { + super( + `convertProseMirrorToMarkdown: unknown ${kind} type "${typeName}" has no lossless markdown representation (strict mode)`, + ); + this.name = "ConverterLossError"; + this.kind = kind; + this.typeName = typeName; + } +} + /** * Options for {@link convertProseMirrorToMarkdown}. */ @@ -46,6 +66,23 @@ export interface ConvertProseMirrorToMarkdownOptions { * path where resolved anchors MUST be preserved for round-tripping. */ dropResolvedCommentAnchors?: boolean; + /** + * Optional sink for LOSS warnings. When the serializer reaches a node or mark + * type it has no dedicated case for, it degrades gracefully (flattens an + * unknown node to its children, drops an unknown mark) — historically a SILENT + * data loss. When this array is provided, one human-readable message per such + * event is pushed here so the caller can observe (and log) what was degraded. + * Not provided by default -> behavior is byte-identical to before for existing + * callers. + */ + warnings?: string[]; + /** + * When true, THROW a {@link ConverterLossError} on the FIRST unknown node/mark + * instead of degrading silently — a warning becomes a hard error. Used by the + * lossless git-sync export path and the converter tests, where an unmapped + * type is a bug to surface, not data to quietly drop. + */ + strict?: boolean; } /** @@ -63,6 +100,70 @@ export interface ConvertProseMirrorToMarkdownOptions { * separator is emitted for any other join, so non-list output is unchanged. */ const LIST_MARKER_SEPARATOR = ""; + +/** + * Backslash-escape a leading markdown BLOCK trigger so a serialized paragraph + * line re-parses as a PARAGRAPH, not another block. Without this, a paragraph + * whose text begins at column 0 with an ATX heading `#`, a blockquote/callout + * `>`, a bullet marker `-`/`*`/`+`, an ordered marker `N.`/`N)`, a code fence + * (```` ``` ````/`~~~`), a table `|`, or a thematic break (`---`/`***`/`___`, + * solid or spaced) silently becomes a heading/list/quote/code block/table/rule + * on the next markdown -> ProseMirror import — a known data-loss class (the + * thematic-break case drops the text entirely, since a horizontalRule carries + * none). CommonMark's escape tokenizer decodes the inserted `\` back to the + * literal character on import AND stops the block interpretation, so the line + * round-trips byte-exact as paragraph text. Only the FIRST offending character + * is escaped (the minimum needed to break block recognition); a line that does + * NOT open a block — emphasis `**x**`, an inline code span, ordinary prose — is + * returned verbatim, so there is no backslash churn for the common case. + * + * Applied ONLY to paragraph text, once per `\n`-separated LINE (the paragraph + * case splits on `\n` — each hardBreak emits ` \n` — so a trigger on a + * continuation line is escaped too): headings/lists/blockquotes legitimately + * open with these markers and render them from their own cases. This is the + * single, canonical fix for the class the client bridge worked around with a + * ZWSP (`gitmost-recording.ts`) and the generative suite self-censored around + * (`text-arbitraries.ts`) — both now removed. + */ +function escapeLeadingBlockTrigger(line: string): string { + // ATX heading: 1..6 `#` then whitespace/EOL. + if (/^#{1,6}(?:\s|$)/.test(line)) return "\\" + line; + // Blockquote / Docmost callout opener (`>` or `> [!info]`). + if (line.startsWith(">")) return "\\" + line; + // Bullet list marker then whitespace/EOL. Emphasis (`*x*`, `**x**`) has no + // space after the leading marker and is intentionally left verbatim. + if (/^[-*+](?:\s|$)/.test(line)) return "\\" + line; + // Ordered list marker `N.` / `N)`: escape the DELIMITER so the digits stay + // literal (`1. x` -> `1\. x`, which imports back as the text `1. x`). + const ordered = line.match(/^(\d+)[.)](?:\s|$)/); + if (ordered) { + const digits = ordered[1].length; + return line.slice(0, digits) + "\\" + line.slice(digits); + } + // Fenced code block: 3+ backticks or tildes. A single/double backtick is an + // inline code span and is left verbatim. + if (/^(?:`{3,}|~{3,})/.test(line)) return "\\" + line; + // Thematic break: a WHOLE line of 3+ identical `-`/`*`/`_`, optionally spaced. + if (/^([-*_])(?:\s*\1){2,}\s*$/.test(line)) return "\\" + line; + // Setext underline: a continuation line (after a hardBreak) that is ONLY `-` + // or ONLY `=` (any count, trailing spaces allowed). Under a paragraph line + // such a line re-parses as a SETEXT HEADING and SILENTLY DROPS its own text + // (`a\n--` -> heading "a", the `--` is LOST; `a\n=` -> heading "a", `=` LOST). + // The bullet arm above catches a lone `-` (via its `$`) and the thematic arm + // catches 3+ dashes, but exactly TWO dashes (`--`) fall through both; and no + // arm covers a lone `=` at all (a `==` pair is neutralized earlier by the + // inline `==`->`\=\=` escape, so only a single `=` line reaches here). Escaping + // the leading char (`\--`, `\=`) breaks the setext interpretation so the line + // round-trips as paragraph text. The WHOLE line must be the marker (anchored + // `^-+`/`^=+` to EOL), so a mid-content `-`/`=` is never spuriously escaped; + // and a `---`/`----` already handled by the thematic arm never reaches here, + // so there is no double-escape. + if (/^-+[ \t]*$/.test(line) || /^=+[ \t]*$/.test(line)) return "\\" + line; + // GFM table row opener. + if (line.startsWith("|")) return "\\" + line; + return line; +} + function listMarkerFamily(type: string | undefined): "ul" | "ol" | null { if (type === "bulletList" || type === "taskList") return "ul"; if (type === "orderedList") return "ol"; @@ -109,6 +210,26 @@ export function convertProseMirrorToMarkdown( // callers (mcp getPage / in-app AI chat) pass it true. const dropResolvedCommentAnchors = options.dropResolvedCommentAnchors === true; + // Loss reporting for node/mark types with no dedicated serializer case. In + // `strict` mode the FIRST such type throws (git-sync, tests); otherwise the + // serializer degrades gracefully (as it always has) but records one warning + // per unmapped type into the optional sink so the loss is observable, not + // silent. Deduped per type so a document with many unknown nodes of one type + // produces one message. + const strict = options.strict === true; + const warningsSink = options.warnings; + const seenLossTypes = new Set(); + const warnLoss = (kind: "node" | "mark", typeName: string): void => { + if (strict) throw new ConverterLossError(kind, typeName); + if (!warningsSink) return; + const key = `${kind}:${typeName}`; + if (seenLossTypes.has(key)) return; + seenLossTypes.add(key); + warningsSink.push( + `Unknown ${kind} type "${typeName}" has no lossless markdown form; it was degraded on export.`, + ); + }; + // Escape a value interpolated into an HTML double-quoted attribute value // (textAlign, colors, image src, math `text`, all data-* attrs, etc.). In the // ATTRIBUTE context only the quote that delimits the value and the ampersand @@ -412,7 +533,17 @@ export function convertProseMirrorToMarkdown( } case "paragraph": { - const text = renderInlineChildren(nodeContent); + // Escape a leading block trigger on EVERY line of the paragraph, not + // just the first: a hardBreak serializes as ` \n`, so a `#`/`-`/`>`/ + // `1.`/`|`/fence/`---` at the start of a CONTINUATION line would also + // re-parse into another block on the next import (a heading/list/table/ + // setext-`---`), and for the text-less thematic/setext case would LOSE + // that line's text entirely. Escaping each `\n`-separated line closes + // the class for multi-line paragraphs too. + const text = renderInlineChildren(nodeContent) + .split("\n") + .map(escapeLeadingBlockTrigger) + .join("\n"); const align = node.attrs?.textAlign; // Non-default alignment round-trips as an ATTACHED HTML comment at the // END of the block line (#293 canon #9): @@ -595,6 +726,12 @@ export function convertProseMirrorToMarkdown( } break; } + default: + // Unknown mark: no dedicated case, so it has no markdown form and + // is dropped from the run. Report the loss (throws in strict + // mode) then leave the text unwrapped — the historical behavior. + warnLoss("mark", String(mark.type)); + break; } } } @@ -1173,7 +1310,11 @@ export function convertProseMirrorToMarkdown( } default: - // Fallback: process children + // Unknown node type: no dedicated case, so the node's identity + attrs + // have no lossless markdown form. Report the loss (throws in strict + // mode) then degrade by flattening to its children — the historical + // graceful fallback. + warnLoss("node", String(type)); return nodeContent.map(processNode).join(""); } }; @@ -1297,6 +1438,12 @@ export function convertProseMirrorToMarkdown( t = `${t}`; } break; + default: + // Unknown mark on the raw-HTML path: dropped (no HTML form). Report + // the loss (throws in strict mode) — same policy as the markdown + // path's marks loop above. + warnLoss("mark", String(mark.type)); + break; } } return t; diff --git a/packages/prosemirror-markdown/src/lib/text-normalize.ts b/packages/prosemirror-markdown/src/lib/text-normalize.ts index ea84d648..5e5ad26c 100644 --- a/packages/prosemirror-markdown/src/lib/text-normalize.ts +++ b/packages/prosemirror-markdown/src/lib/text-normalize.ts @@ -7,13 +7,12 @@ * it is never applied to replacement text or inserted node content, so no * formatting is ever lost. * - * Scope note (#414): this package-local copy exists so `node-ops.ts` — which - * lives here now (the single canonical copy) — can resolve its markdown-tolerant - * anchor fallback without a circular dependency back on `@docmost/mcp`. It - * intentionally carries ONLY `stripInlineMarkdown` (the primitive `node-ops` - * needs); the mcp-side `text-normalize.ts` (which additionally serves - * `json-edit.ts` via `stripBalancedWrappers`) is the subject of a separate - * dedup task and is left untouched here. + * CANONICAL HOME (#414/#493): this is the single source of truth for locator + * markdown-stripping. `node-ops.ts` (which lives here) uses it directly, and the + * mcp-side `text-normalize.ts` now IMPORTS `stripInlineMarkdown` and the shared + * `stripWrappersAndLinks` primitive from here (via `@docmost/prosemirror-markdown`) + * instead of keeping a drifting copy — mcp only adds its own thin + * `stripBalancedWrappers`/`closestBlockHint` on top. */ /** Maximum unwrap passes, so pathological/nested input cannot loop forever. */ @@ -44,7 +43,7 @@ const LINK_IMAGE_RE = /!?\[([^\]]*)\]\([^)]*\)/g; * Does NOT trim decoration, does NOT guard against an empty result — it returns * exactly the transformed string. */ -function stripWrappersAndLinks(s: string): string { +export function stripWrappersAndLinks(s: string): string { // 1. Links/images -> their visible text. let out = s.replace(LINK_IMAGE_RE, "$1"); diff --git a/packages/prosemirror-markdown/test/converter-loss-warnings.test.ts b/packages/prosemirror-markdown/test/converter-loss-warnings.test.ts new file mode 100644 index 00000000..fc6e22b8 --- /dev/null +++ b/packages/prosemirror-markdown/test/converter-loss-warnings.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from "vitest"; +import { + convertProseMirrorToMarkdown, + ConverterLossError, +} from "../src/lib/markdown-converter.js"; + +/** + * #493 commit 3 — a node/mark type the serializer has no dedicated case for used + * to be degraded SILENTLY (an unknown node flattened to its children, an unknown + * mark dropped from the run). The serializer now REPORTS the loss: + * - default (non-strict): unchanged graceful degradation, but one warning per + * unmapped type is pushed into an optional `warnings` sink so callers can + * observe it; + * - strict: the FIRST unmapped type throws a ConverterLossError (git-sync + + * tests), turning a silent loss into a hard, surfaced error. + * + * Exercised through the REAL converter (no mock): the observable properties are + * the emitted markdown, the warnings collected, and the thrown error. + */ + +const doc = (...nodes: any[]) => ({ type: "doc", content: nodes }); + +describe("converter loss reporting — unknown node types", () => { + const unknownNode = doc({ + type: "quantumWidget", + content: [{ type: "text", text: "inner text" }], + }); + + it("degrades to children AND records a warning (non-strict, sink provided)", () => { + const warnings: string[] = []; + const md = convertProseMirrorToMarkdown(unknownNode, { warnings }); + // Graceful degrade: the child text still survives (historical behavior). + expect(md).toContain("inner text"); + // The loss is now observable. + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("quantumWidget"); + expect(warnings[0]).toContain("node"); + }); + + it("stays byte-identical for callers that pass no sink (zero behavior change)", () => { + const withSink: string[] = []; + const a = convertProseMirrorToMarkdown(unknownNode, { warnings: withSink }); + const b = convertProseMirrorToMarkdown(unknownNode); + expect(b).toBe(a); // the sink does not alter the produced markdown + }); + + it("throws ConverterLossError in strict mode", () => { + try { + convertProseMirrorToMarkdown(unknownNode, { strict: true }); + expect.unreachable("strict mode must throw on an unknown node"); + } catch (e) { + expect(e).toBeInstanceOf(ConverterLossError); + expect((e as ConverterLossError).kind).toBe("node"); + expect((e as ConverterLossError).typeName).toBe("quantumWidget"); + } + }); + + it("dedupes the warning per type (many unknown nodes -> one message)", () => { + const warnings: string[] = []; + convertProseMirrorToMarkdown( + doc( + { type: "quantumWidget", content: [{ type: "text", text: "a" }] }, + { type: "quantumWidget", content: [{ type: "text", text: "b" }] }, + ), + { warnings }, + ); + expect(warnings).toHaveLength(1); + }); +}); + +describe("converter loss reporting — unknown mark types", () => { + const unknownMark = doc({ + type: "paragraph", + content: [{ type: "text", text: "glowing", marks: [{ type: "glow" }] }], + }); + + it("drops the mark but keeps the text AND records a warning (non-strict)", () => { + const warnings: string[] = []; + const md = convertProseMirrorToMarkdown(unknownMark, { warnings }); + expect(md).toBe("glowing"); // text survives, mark silently had no form + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("glow"); + expect(warnings[0]).toContain("mark"); + }); + + it("throws ConverterLossError in strict mode", () => { + expect(() => + convertProseMirrorToMarkdown(unknownMark, { strict: true }), + ).toThrow(ConverterLossError); + }); +}); + +describe("converter loss reporting — known content is never flagged", () => { + it("a fully-mapped document produces no warnings and does not throw in strict mode", () => { + const d = doc( + { type: "heading", attrs: { level: 2 }, content: [{ type: "text", text: "Title" }] }, + { + type: "paragraph", + content: [ + { type: "text", text: "bold", marks: [{ type: "bold" }] }, + { type: "text", text: " and " }, + { type: "text", text: "link", marks: [{ type: "link", attrs: { href: "https://x.y" } }] }, + ], + }, + { type: "bulletList", content: [{ type: "listItem", content: [{ type: "paragraph", content: [{ type: "text", text: "item" }] }] }] }, + ); + const warnings: string[] = []; + const md = convertProseMirrorToMarkdown(d, { warnings, strict: true }); + expect(warnings).toEqual([]); + expect(md).toContain("## Title"); + }); +}); diff --git a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts b/packages/prosemirror-markdown/test/foreign-markdown.test.ts similarity index 75% rename from apps/server/src/integrations/import/utils/foreign-markdown.spec.ts rename to packages/prosemirror-markdown/test/foreign-markdown.test.ts index 8a43fa20..135233fb 100644 --- a/apps/server/src/integrations/import/utils/foreign-markdown.spec.ts +++ b/packages/prosemirror-markdown/test/foreign-markdown.test.ts @@ -1,12 +1,15 @@ +import { describe, it, expect } from 'vitest'; +import { convertProseMirrorToMarkdown } from '../src/lib/markdown-converter.js'; +import { markdownToProseMirror } from '../src/lib/markdown-to-prosemirror.js'; import { - convertProseMirrorToMarkdown, - markdownToProseMirror, -} from '@docmost/prosemirror-markdown'; -import { normalizeForeignMarkdown } from './foreign-markdown'; + normalizeForeignMarkdown, + normalizeAgentMarkdown, +} from '../src/lib/foreign-markdown.js'; /** - * STEP 2 goldens for issue #345: the foreign-markdown normalizer that runs at the - * import boundary BEFORE the strict canonical parser (`markdownToProseMirror`). + * STEP 2 goldens for issue #345 (moved into the package with the normalizer in + * #493): the foreign-markdown normalizer that runs at the import boundary BEFORE + * the strict canonical parser (`markdownToProseMirror`). * * Two layers: * 1. PURE string→string cases pinning the normalizer's own behavior (GFM @@ -216,3 +219,53 @@ describe('foreign markdown import acceptance (normalizer + canonical parser)', ( ).toHaveLength(1); }); }); + +describe('normalizeAgentMarkdown vs normalizeForeignMarkdown — front-matter strip is IMPORT-only (#493 review)', () => { + // A page that OPENS with a horizontalRule and contains a later `---` serializes + // to a `---…---`-shaped body. On a full-body AGENT rewrite this must NOT be + // mistaken for YAML front-matter and stripped — that silently dropped the + // page's leading content. + const rulePage = '---\n\nIntro\n\nMore\n\n---\n\nRest'; + + it('normalizeAgentMarkdown does NOT strip a leading ---…--- (no content loss)', () => { + expect(normalizeAgentMarkdown(rulePage)).toBe(rulePage); + }); + + it('normalizeForeignMarkdown (file import) STILL strips a real leading YAML front-matter block', () => { + const withYaml = '---\ntitle: My Page\ntags: [a, b]\n---\n\nBody here.'; + const out = normalizeForeignMarkdown(withYaml); + expect(out).toBe('Body here.'); + // And the horizontalRule-shaped body IS stripped on the import path (its + // documented file-import behavior) — the two variants differ ONLY here. + expect(normalizeForeignMarkdown(rulePage)).not.toContain('Intro'); + }); + + it('agent-write round-trip keeps a horizontalRule-led doc with a second rule intact', async () => { + // Simulate the serializer output for [horizontalRule, para, para, horizontalRule, para]. + const doc = { + type: 'doc', + content: [ + { type: 'horizontalRule' }, + { type: 'paragraph', content: [{ type: 'text', text: 'Intro' }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'More' }] }, + { type: 'horizontalRule' }, + { type: 'paragraph', content: [{ type: 'text', text: 'Rest' }] }, + ], + }; + const body = convertProseMirrorToMarkdown(doc); + // The agent-write normalization must NOT eat the head; re-import keeps every + // paragraph's text. + const back = await markdownToProseMirror(normalizeAgentMarkdown(body)); + const texts = JSON.stringify(back); + for (const t of ['Intro', 'More', 'Rest']) expect(texts).toContain(t); + // Both horizontal rules survive. + expect(back.content.filter((n: any) => n.type === 'horizontalRule')).toHaveLength(2); + }); + + it('agent-write STILL rewrites GFM reference footnotes (the shared drift-fix)', () => { + const gfm = 'See[^1].\n\n[^1]: the note.'; + const out = normalizeAgentMarkdown(gfm); + expect(out).toContain('^[the note.]'); + expect(out).not.toMatch(/\[\^1\]:/); + }); +}); diff --git a/packages/prosemirror-markdown/test/generative/text-arbitraries.ts b/packages/prosemirror-markdown/test/generative/text-arbitraries.ts index 76353b76..63067427 100644 --- a/packages/prosemirror-markdown/test/generative/text-arbitraries.ts +++ b/packages/prosemirror-markdown/test/generative/text-arbitraries.ts @@ -212,25 +212,93 @@ export function normalizeInline(nodes: any[]): any[] { return out; } +/** + * #493 commit 1: a plain-text run whose text DELIBERATELY OPENS with a markdown + * BLOCK trigger — ATX heading `#`, bullet `-`/`*`/`+`, blockquote `>`, ordered + * `N.`/`N)`, or a table `|` — followed by safe text. Pre-#493 the corpus + * self-censored these away (safeTextArb's leading-word guarantee); the paragraph + * serializer now BLOCK-ESCAPES a leading trigger, so the generative round-trip + * itself proves the data-loss class is closed rather than avoiding it. + * + * DELIBERATELY excludes the code-fence (backtick) trigger — the backtick is a + * code-span delimiter that re-pairs globally (see specialCharArb's note), an + * instability UNRELATED to block-escape — and the whole-line thematic break + * (`---`), which only triggers when the line is ONLY dashes; both are covered by + * the deterministic pin (gitmost-transcript-neutralization.test.ts). Each still + * ENDS in a word (safeTextArb) so adjacent-run concatenation stays safe. + */ +export const blockTriggerLeadRunArb: fc.Arbitrary = fc + .tuple( + fc.constantFrom('# ', '## ', '- ', '* ', '+ ', '> ', '1. ', '1) ', '| '), + safeTextArb, + ) + .map(([trigger, rest]) => ({ type: 'text', text: trigger + rest })); + +/** + * A hardBreak IMMEDIATELY followed by a block-trigger-leading run — a two-node + * segment. Because a hardBreak serializes as ` \n`, the trigger then sits at + * the START of a CONTINUATION line, exercising the serializer's PER-LINE block + * escape (not just the first line). #493 review: without this the fuzzer never + * placed a trigger after a hardBreak, so a single-line-only escape passed P1–P3. + */ +export const hardBreakThenTriggerArb: fc.Arbitrary = fc + .tuple(hardBreakArb, blockTriggerLeadRunArb) + .map(([hb, trigger]) => [hb, trigger]); + +/** + * #493 (setext data-loss): a WHOLE-LINE setext underline landing on a + * continuation line. A setext underline is a line of ONLY `-` (any count) or + * ONLY `=` (any count) that FOLLOWS a paragraph line; on re-parse it turns the + * preceding line into a heading and DROPS its own text. The block-escape must + * neutralize it. Unlike blockTriggerLeadRunArb, the underline must occupy the + * whole line, so we sandwich it between two hardBreaks (underline on its own + * line, preceded by earlier paragraph content, followed by a trailing word so + * the closing hardBreak is not dropped by normalizeInline). Covers underlines + * of every length: `--` (the two-dash case the bullet/thematic arms miss), a + * lone `=`, `==`/`====` (neutralized by the inline `==` escape), and `---`/ + * `----` (regression for the existing thematic case). + */ +export const hardBreakThenSetextArb: fc.Arbitrary = fc + .tuple( + fc.constantFrom('--', '=', '==', '====', '---', '----'), + safeTextArb, + ) + .map(([underline, rest]) => [ + { type: 'hardBreak' }, + { type: 'text', text: underline }, + { type: 'hardBreak' }, + { type: 'text', text: rest }, + ]); + /** * Inline content for a paragraph: at least one marked text run, optionally with - * inline atoms (math/mention) and hard breaks interspersed. Always starts with a - * text run so the paragraph never opens with a block trigger. (Ported.) + * inline atoms (math/mention) and hard breaks interspersed. The FIRST run is + * usually an ordinary marked run, but sometimes a block-trigger-leading run + * (blockTriggerLeadRunArb) so the paragraph OPENS with a markdown block trigger; + * and a `hardBreak + trigger` segment can appear anywhere in the rest, so a + * trigger also lands at the start of a CONTINUATION line — both exercising the + * serializer's per-line block-escape end-to-end. (Ported, with the #493 + * leading-trigger + post-hardBreak dimensions added.) */ export const inlineContentArb: fc.Arbitrary = fc .tuple( - markedTextRunArb, + fc.oneof( + { weight: 5, arbitrary: markedTextRunArb }, + { weight: 1, arbitrary: blockTriggerLeadRunArb }, + ), fc.array( fc.oneof( - { weight: 5, arbitrary: markedTextRunArb }, - { weight: 1, arbitrary: mathInlineArb }, - { weight: 1, arbitrary: mentionArb }, - { weight: 1, arbitrary: hardBreakArb }, + { weight: 5, arbitrary: markedTextRunArb.map((n) => [n]) }, + { weight: 1, arbitrary: mathInlineArb.map((n) => [n]) }, + { weight: 1, arbitrary: mentionArb.map((n) => [n]) }, + { weight: 1, arbitrary: hardBreakArb.map((n) => [n]) }, + { weight: 2, arbitrary: hardBreakThenTriggerArb }, + { weight: 2, arbitrary: hardBreakThenSetextArb }, ), { minLength: 0, maxLength: 4 }, ), ) - .map(([first, rest]) => normalizeInline([first, ...rest])); + .map(([first, rest]) => normalizeInline([first, ...rest.flat()])); /** * Inline content for a HEADING — identical to a paragraph's, but WITHOUT hard diff --git a/packages/prosemirror-markdown/test/gitmost-transcript-neutralization.test.ts b/packages/prosemirror-markdown/test/gitmost-transcript-neutralization.test.ts index 6fb35f1f..20b9f092 100644 --- a/packages/prosemirror-markdown/test/gitmost-transcript-neutralization.test.ts +++ b/packages/prosemirror-markdown/test/gitmost-transcript-neutralization.test.ts @@ -5,32 +5,21 @@ import { convertProseMirrorToMarkdown } from "../src/lib/markdown-converter.js"; import { markdownToProseMirror } from "../src/lib/markdown-to-prosemirror.js"; /** - * gitmost #377 (round-1 review, finding #1) — proof, against the REAL - * converter, that the transcript-insert boundary defense survives git-sync. + * #493 commit 1 — the paragraph serializer's leading-block-escape closes the + * data-loss class where a paragraph whose text opens at column 0 with a markdown + * block trigger (`#`/`-`/`*`/`+`/`>`, an ordered `N.`/`N)`, a code fence, a + * table `|`, a callout opener, or a thematic break) silently re-parsed into a + * heading / list / quote / code block / table / horizontalRule on the git-sync + * doc -> markdown -> doc cycle. The thematic-break case was the worst: a + * horizontalRule carries NO text, so the line's text was lost entirely. * - * The web bridge (apps/client .../gitmost/gitmost-recording.ts, - * `gitmostInsertTranscriptIntoEditor`) appends each transcript line as a - * PARAGRAPH text node. The paragraph serializer here (`case "paragraph"`) emits - * that text VERBATIM with no block-escape, so a line whose text begins with a - * col-0 markdown block trigger would, on the doc -> markdown -> doc git-sync - * cycle, silently re-parse into a heading / list / quote / callout / code block. - * That missing block-escape is the pre-existing root cause; the bridge's - * boundary defense prepends an invisible zero-width space (U+200B) to a line - * that begins with such a trigger, shifting it off column 0. - * - * This test keeps a COPY of the bridge's trigger regex (the bridge is in a - * different package and can't be imported here) and asserts: - * 1. bare trigger lines DO corrupt (documents the root cause), and - * 2. the ZWSP-neutralized form round-trips as a single PARAGRAPH with the - * text byte-preserved. + * This is the deterministic PIN, one assertion per trigger, exercised through + * the REAL converter round-trip (not a mock): each bare trigger line now + * round-trips as a SINGLE paragraph with its text byte-preserved — proving the + * class is closed WITHOUT the former client-side ZWSP workaround (removed) or + * the generative suite's leading-word self-censorship (removed). */ -const ZWSP = "​"; // U+200B - -// MUST stay in sync with GITMOST_MD_BLOCK_TRIGGER_RE in the client bridge. -const MD_BLOCK_TRIGGER_RE = - /^(?:#{1,6}(?:\s|$)|[-*+](?:\s|$)|>|\d+[.)](?:\s|$)|```|~~~|\||([-*_])(?:\s*\1){2,}\s*$)/; - const doc = (...nodes: any[]) => ({ type: "doc", content: nodes }); const para = (t: string) => ({ type: "paragraph", @@ -43,78 +32,117 @@ const roundtrip = async (text: string) => { return back.content as any[]; }; -describe("gitmost transcript neutralization (git-sync round-trip)", () => { - // Lines that, at column 0, the serializer's missing block-escape would let - // git-sync re-parse into a non-paragraph block. +describe("paragraph block-escape (git-sync round-trip)", () => { + // Every line here, at column 0, WOULD (pre-fix) re-parse into a non-paragraph + // block. Each is now block-escaped by the serializer and round-trips clean. const triggerLines = [ "- dash", "* star", "+ plus", "> quote", "# hash", + "## two hash", + "###### six hash", "1. one", "1) one", "> [!info] note", "```js", "~~~", - // Solid + spaced thematic breaks — these re-parse into a `horizontalRule`, - // which carries NO text, so a bare separator line LOSES its text entirely - // (round-2 finding). `_` also only forms a block via this construct. + "| a | b |", + // Solid + spaced thematic breaks — the text-LOSING case pre-fix. "---", "***", "___", - "- - -", // spaced dash break (solid form is caught by [-*+]\s too, but this is the break) + "- - -", "_ _ _", ]; - it("BARE trigger lines corrupt into non-paragraph blocks (root cause)", async () => { + it("every bare trigger line round-trips as a single paragraph, text byte-preserved", async () => { for (const line of triggerLines) { const blocks = await roundtrip(line); - // At least one produced block is NOT a paragraph — i.e. corruption. - const allParagraphs = blocks.every((b) => b.type === "paragraph"); - expect( - allParagraphs, - `expected "${line}" to corrupt when inserted bare`, - ).toBe(false); - } - }); - - it("BARE solid thematic breaks corrupt into a text-LOSING horizontalRule", async () => { - // The severe case: no text node survives. Documents why neutralization - // matters more here than for list/quote (where the text survived). - for (const line of ["---", "***", "___"]) { - const blocks = await roundtrip(line); - expect(blocks.map((b) => b.type)).toContain("horizontalRule"); - // No block carries the original text anywhere. - const flat = JSON.stringify(blocks); - expect(flat).not.toContain(line); - } - }); - - it("ZWSP-neutralized trigger lines round-trip as a single paragraph, text preserved", async () => { - for (const line of triggerLines) { - // The regex must actually classify each as a trigger. - expect(MD_BLOCK_TRIGGER_RE.test(line), `regex missed "${line}"`).toBe( - true, + expect(blocks, `"${line}" should be one block`).toHaveLength(1); + expect(blocks[0].type, `"${line}" should stay a paragraph`).toBe( + "paragraph", ); - const neutralized = ZWSP + line; - const blocks = await roundtrip(neutralized); - - expect(blocks).toHaveLength(1); - expect(blocks[0].type).toBe("paragraph"); - // Text is byte-preserved (ZWSP + original line), so the display is the - // original line with only an invisible leading character. - expect(blocks[0].content[0].text).toBe(neutralized); + expect( + blocks[0].content?.[0]?.text, + `"${line}" text should survive byte-exact`, + ).toBe(line); } }); - it("normal host-prefixed lines never match the trigger regex and round-trip byte-exact", async () => { + it("emphasis / inline-code paragraphs are NOT escaped (no backslash churn)", async () => { + // These open with `*`/`` ` `` but are NOT block triggers; the serialized + // markdown must not gain a stray leading backslash, and they round-trip. + for (const [text, mark] of [ + ["bold", "bold"], + ["italic", "italic"], + ["code", "code"], + ] as const) { + const node = doc({ + type: "paragraph", + content: [{ type: "text", text, marks: [{ type: mark }] }], + }); + const md = convertProseMirrorToMarkdown(node); + expect(md.startsWith("\\"), `${mark} must not be block-escaped`).toBe( + false, + ); + const back = await markdownToProseMirror(md); + expect(back.content[0].type).toBe("paragraph"); + expect(back.content[0].content[0].text).toBe(text); + expect(back.content[0].content[0].marks?.[0]?.type).toBe(mark); + } + }); + + it("a block trigger on a CONTINUATION line (after a hardBreak) is escaped too", async () => { + // A hardBreak serializes as ` \n`, so a trigger on the second line would, + // without a per-line escape, re-parse into another block. The worst case is + // `---`: a setext underline would turn the first line into a heading and LOSE + // the `---` text entirely. Each pair round-trips as ONE paragraph with the + // hardBreak and both texts preserved. + for (const [first, second] of [ + ["a", "# b"], + ["a", "- b"], + ["a", "> b"], + ["a", "1. b"], + ["a", "| b |"], + ["a", "---"], // setext / thematic (3 dashes) — the text-losing case + ["a", "--"], // setext underline, EXACTLY two dashes (bullet/thematic miss it) + ["a", "----"], // setext / thematic (4 dashes) + ["a", "="], // setext H1 underline, a lone `=` (no other arm covers it) + ["a", "===="], // setext H1 underline, run of `=` + ]) { + const d = doc({ + type: "paragraph", + content: [ + { type: "text", text: first }, + { type: "hardBreak" }, + { type: "text", text: second }, + ], + }); + const back = await markdownToProseMirror(convertProseMirrorToMarkdown(d)); + expect(back.content, `"${first}⏎${second}" should be one block`).toHaveLength(1); + expect(back.content[0].type).toBe("paragraph"); + const texts = (back.content[0].content as any[]) + .filter((n) => n.type === "text") + .map((n) => n.text); + const hasBreak = (back.content[0].content as any[]).some( + (n) => n.type === "hardBreak", + ); + expect(hasBreak, `"${first}⏎${second}" should keep the hardBreak`).toBe(true); + expect(texts, `"${first}⏎${second}" should preserve both line texts`).toEqual([ + first, + second, + ]); + } + }); + + it("normal host-prefixed lines round-trip byte-exact (unaffected)", async () => { for (const line of [ "You: hello there", "Speaker 1: - and then a dash mid-line", "Speaker 2: 1. not a list", ]) { - expect(MD_BLOCK_TRIGGER_RE.test(line)).toBe(false); const blocks = await roundtrip(line); expect(blocks).toHaveLength(1); expect(blocks[0].type).toBe("paragraph"); diff --git a/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts b/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts index c3bb1e2b..9008c34b 100644 --- a/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts +++ b/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts @@ -430,7 +430,7 @@ describe('converter gap coverage — emission branches (specs 1–11)', () => { }); }); -describe('converter gap coverage — documented round-trip data loss (specs 12–14)', () => { +describe('converter gap coverage — formerly-lossy round-trips, now closed (specs 12–14)', () => { // 12. A 3-backtick fence inside a codeBlock body is now lengthened: the outer // fence widens to (longest inner run + 1) backticks per CommonMark, so the // inner ``` is treated as content and the block survives as ONE node. @@ -460,25 +460,24 @@ describe('converter gap coverage — documented round-trip data loss (specs 12 expect(docsCanonicallyEqual(d, doc2)).toBe(false); }); - // 13. A leading ordered-list marker in paragraph text is NOT escaped, so a - // plain paragraph silently becomes an orderedList on re-import. - it('a paragraph starting with "1. " is promoted to an orderedList on re-import', async () => { + // 13. #493 commit 1: a leading ordered-list marker in paragraph text is now + // BLOCK-ESCAPED, so the paragraph round-trips as a paragraph instead of + // silently becoming an orderedList (was documented data loss, now closed). + it('a paragraph starting with "1. " is block-escaped and stays a paragraph', async () => { const d = doc({ type: 'paragraph', content: [{ type: 'text', text: '1. not a list' }], }); const md1 = convertProseMirrorToMarkdown(d); - expect(md1).toBe('1. not a list'); // no backslash escape + expect(md1).toBe('1\\. not a list'); // the ordered-list delimiter is escaped const doc2 = await markdownToProseMirror(md1); - expect(doc2.content?.[0]?.type).toBe('orderedList'); - const li = doc2.content[0].content?.[0]; - expect(li?.type).toBe('listItem'); - expect(li.content?.[0]?.content?.[0]).toMatchObject({ + expect(doc2.content?.[0]?.type).toBe('paragraph'); + expect(doc2.content[0].content?.[0]).toMatchObject({ type: 'text', - text: 'not a list', // the "1. " was consumed as a list marker + text: '1. not a list', // the escape decodes back to the literal text }); - expect(docsCanonicallyEqual(d, doc2)).toBe(false); + expect(docsCanonicallyEqual(d, doc2)).toBe(true); }); // 14. #293 canon #4: the image title now round-trips via the attached diff --git a/packages/prosemirror-markdown/test/schema-editor-ext-contract.test.ts b/packages/prosemirror-markdown/test/schema-editor-ext-contract.test.ts index 4778ea5c..7570ff11 100644 --- a/packages/prosemirror-markdown/test/schema-editor-ext-contract.test.ts +++ b/packages/prosemirror-markdown/test/schema-editor-ext-contract.test.ts @@ -16,14 +16,16 @@ import * as editorExt from "@docmost/editor-ext"; // or mark added upstream that the mirror forgets to vendor fails CI loudly // (otherwise it is silently dropped on the markdown <-> ProseMirror round-trip). // -// LIMITATION (intentional, see schema-surface-snapshot.test.ts): this is a -// NAME-LEVEL contract only, not a full attribute-level structural compare. -// editor-ext's Tiptap representation (node views, commands, suggestion plugins, -// addGlobalAttributes spread across separate extensions) differs from this -// minimal mirror, so a mechanical attribute-by-attribute equality would be -// fragile and produce false drift. Attribute parity is guarded by the inline -// surface snapshot (reviewed in every diff); this test guards that no canonical -// node/mark TYPE goes unmirrored. StarterKit-provided types (paragraph, bold, +// This file now holds TWO contracts (see the two describe blocks): the original +// NAME-LEVEL type contract (no canonical node/mark TYPE goes unmirrored) AND, as +// of #493, an ATTRIBUTE-LEVEL contract that compares each editor-ext node/mark's +// OWN declared attributes (names + defaults) against the mirror's built schema. +// A full mechanical attribute-by-attribute EQUALITY would be fragile (the mirror +// is a deliberate superset: it injects the global id/textAlign/indent attrs and +// normalizes some editor-ext defaults to null), so the attribute contract is +// asymmetric — editor-ext -> mirror — with a small, reasoned, stale-guarded +// allowlist for the two blessed divergence kinds (non-round-trippable omissions +// and null-normalized defaults). StarterKit-provided types (paragraph, bold, // heading, …) are contributed by @tiptap/starter-kit in the mirror rather than // by editor-ext, so they are naturally covered by the mirror's superset. // @@ -85,3 +87,191 @@ describe("docmost schema vs @docmost/editor-ext (name-level contract)", () => { expect(missing).toEqual([]); }); }); + +// ── ATTRIBUTE-LEVEL CONTRACT (#493 commit 2) ──────────────────────────────── +// +// The name-level contract above catches a WHOLE node/mark type going unmirrored, +// but not ATTRIBUTE drift within a vendored type — the exact class that silently +// dropped `subpages.recursive`: editor-ext grew an attribute the hand-synced +// mirror forgot, so documents using it lost that attribute on a git-sync +// round-trip while CI stayed green. This closes that gap by comparing each +// editor-ext node/mark's OWN declared attributes (names + defaults) against the +// mirror's built ProseMirror schema `spec.attrs`. +// +// DIRECTION: editor-ext -> mirror. The mirror is deliberately a SUPERSET (it +// injects the global `id`/`textAlign`/`indent` attributes and normalizes some +// editor-ext "required" attrs to a `null` default), so a reverse compare would +// be pure false drift; the meaningful failure is an editor-ext attribute the +// mirror DROPS (name) or whose DEFAULT it silently changes. Both directions of +// staleness are guarded so the allowlists cannot rot. + +/** + * The attributes an editor-ext Tiptap Node/Mark DECLARES itself, read from its + * `config.addAttributes()`. Global attributes injected by separate extensions + * (unique-id, indent, textAlign) are NOT included here — they are the mirror's + * superset and are not part of a per-type declaration — so this isolates each + * type's own contribution. A declared attribute with no explicit `default` is a + * required attr (Tiptap default `undefined`); we surface that as-is so the + * default compare can skip it (the mirror makes such attrs optional/`null`). + */ +function editorExtOwnAttrs(): Map< + string, + { kind: "node" | "mark"; attrs: Record } +> { + const out = new Map< + string, + { kind: "node" | "mark"; attrs: Record } + >(); + for (const value of Object.values(editorExt)) { + if (!isTiptapNodeOrMark(value)) continue; + const ext = value as unknown as { + name: string; + type: "node" | "mark"; + options?: unknown; + storage?: unknown; + config?: { addAttributes?: () => Record }; + }; + const fn = ext.config?.addAttributes; + // addAttributes reads `this.options`/`this.name`; bind a minimal context + // (verified sufficient for every editor-ext extension — none reach for + // `this.editor` here). A type with no addAttributes contributes no attrs. + const declared = + typeof fn === "function" + ? fn.call({ + options: ext.options ?? {}, + name: ext.name, + parent: undefined, + storage: ext.storage ?? {}, + } as never) + : {}; + const attrs: Record = {}; + for (const [attr, spec] of Object.entries(declared || {})) { + // `undefined` marks a required (no-default) attr; keep it so the default + // compare can distinguish "no default declared" from "default is null". + attrs[attr] = (spec as { default?: unknown })?.default; + } + out.set(ext.name, { kind: ext.type, attrs }); + } + return out; +} + +/** The mirror's built-schema `spec.attrs` for a type: attr name -> default. */ +function mirrorAttrs( + name: string, + kind: "node" | "mark", +): Record | null { + const schema = getSchema(docmostExtensions as never); + const spec = kind === "node" ? schema.nodes[name]?.spec : schema.marks[name]?.spec; + if (!spec) return null; + const out: Record = {}; + for (const [attr, def] of Object.entries(spec.attrs || {})) { + out[attr] = (def as { default?: unknown }).default; + } + return out; +} + +// An editor-ext attribute the mirror deliberately does NOT vendor because it has +// NO markdown round-trip representation — dropping it loses nothing on the +// git-sync cycle (the same rationale the flat-roundtrip property suite uses to +// allowlist e.g. `tableCell.backgroundColorName`). Blessed by the hand-curated +// surface snapshot (schema-surface-snapshot.test.ts), reviewed in every diff. +const ACCEPTED_ATTR_OMISSIONS = new Set([ + "highlight.colorName", // only `highlight.color` round-trips (==text==); the + // secondary palette-name is presentational and has no markdown form. +]); + +// An editor-ext attribute the mirror vendors but with a DIFFERENT default: the +// mirror normalizes an "absent" value to `null` (its uniform optional-attr +// convention) rather than editor-ext's UI-oriented default. None of these attrs +// is emitted on the markdown surface (the converter round-trips only the +// serializable ones), so the default never round-trips and the divergence is +// inert — but pinned here so a NEW default change on either side forces review. +const ACCEPTED_DEFAULT_DIVERGENCE = new Set([ + "image.src", // mirror null vs editor "" (an image is never emitted src-less) + "link.internal", // mirror null vs editor false (routing attr, not in md link) + "pdf.width", // mirror null vs editor 800 (presentational sizing, not in md) + "pdf.height", // mirror null vs editor 600 (presentational sizing, not in md) +]); + +describe("docmost schema vs @docmost/editor-ext (attribute-level contract)", () => { + it("vendors every editor-ext attribute (name) of every shared type — no silently-dropped attrs", () => { + const dropped: string[] = []; + for (const [name, { kind, attrs }] of editorExtOwnAttrs()) { + const mirror = mirrorAttrs(name, kind); + if (!mirror) continue; // whole-type omission is the name-level test's job + for (const attr of Object.keys(attrs)) { + const key = `${name}.${attr}`; + if (!(attr in mirror) && !ACCEPTED_ATTR_OMISSIONS.has(key)) { + dropped.push(key); + } + } + } + // Any entry here exists on the editor-ext node/mark but NOT in the mirror + // (and is not a blessed non-round-trippable omission): documents using it + // lose that attribute on a git-sync round-trip — the subpages.recursive + // class. Re-sync src/lib/docmost-schema.ts (and the surface snapshot) or add + // a reasoned ACCEPTED_ATTR_OMISSIONS entry before clearing. + expect(dropped.sort()).toEqual([]); + }); + + it("keeps every editor-ext attribute DEFAULT in sync — no silent default drift", () => { + const drift: string[] = []; + for (const [name, { kind, attrs }] of editorExtOwnAttrs()) { + const mirror = mirrorAttrs(name, kind); + if (!mirror) continue; + for (const [attr, extDefault] of Object.entries(attrs)) { + const key = `${name}.${attr}`; + // Skip attrs editor-ext declares WITHOUT a default (required attrs): + // the mirror deliberately makes them optional (`null`), a safe superset. + if (extDefault === undefined) continue; + if (!(attr in mirror)) continue; // a drop, reported by the name test + if ( + JSON.stringify(mirror[attr]) !== JSON.stringify(extDefault) && + !ACCEPTED_DEFAULT_DIVERGENCE.has(key) + ) { + drift.push( + `${key}: mirror=${JSON.stringify(mirror[attr])} editor-ext=${JSON.stringify(extDefault)}`, + ); + } + } + } + expect(drift.sort()).toEqual([]); + }); + + it("the attribute allowlists have no stale rows (each is really omitted / divergent)", () => { + const ext = editorExtOwnAttrs(); + const staleOmission: string[] = []; + for (const key of ACCEPTED_ATTR_OMISSIONS) { + const [name, attr] = key.split("."); + const entry = ext.get(name); + const mirror = entry ? mirrorAttrs(name, entry.kind) : null; + // Stale if editor-ext no longer declares it, or the mirror now DOES vendor + // it (so it should be removed from the omission allowlist). + if (!entry || !(attr in entry.attrs) || (mirror && attr in mirror)) { + staleOmission.push(key); + } + } + expect(staleOmission, "stale ACCEPTED_ATTR_OMISSIONS rows").toEqual([]); + + const staleDivergence: string[] = []; + for (const key of ACCEPTED_DEFAULT_DIVERGENCE) { + const [name, attr] = key.split("."); + const entry = ext.get(name); + const mirror = entry ? mirrorAttrs(name, entry.kind) : null; + const extDefault = entry?.attrs[attr]; + // Stale if the divergence no longer exists (attr gone, or defaults now + // agree) — the row should be dropped so the allowlist stays honest. + if ( + !entry || + !mirror || + !(attr in entry.attrs) || + !(attr in mirror) || + extDefault === undefined || + JSON.stringify(mirror[attr]) === JSON.stringify(extDefault) + ) { + staleDivergence.push(key); + } + } + expect(staleDivergence, "stale ACCEPTED_DEFAULT_DIVERGENCE rows").toEqual([]); + }); +});