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/prosemirror-markdown/src/lib/index.ts b/packages/prosemirror-markdown/src/lib/index.ts index cbf8f462..062751a4 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 { diff --git a/packages/prosemirror-markdown/src/lib/markdown-converter.ts b/packages/prosemirror-markdown/src/lib/markdown-converter.ts index 0ee31d39..db6a1520 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; } /** @@ -157,6 +194,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 @@ -646,6 +703,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; } } } @@ -1224,7 +1287,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(""); } }; @@ -1348,6 +1415,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/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"); + }); +});