From 54f0ba681ed03be68337430d1e437a7933976243 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Mon, 6 Jul 2026 19:29:03 +0300 Subject: [PATCH 1/4] fix(converter): preserve orderedList start; fuzz column.width numerically (#351) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 3 of the #351 remainder — resolve the two pinned converter counterexamples, fixtures-first. orderedList.start (genuine P1 loss): the converter always emitted "1." and dropped attrs.start. Now the markdown path emits `${start + index}.` (Number-coerced, guards a stray non-numeric start) and the raw-HTML path emits `
    ` when start>1; a default (start=1) list is byte-unchanged. tiptap StarterKit reads both forms back. Un-pinned as a passing regression test; orderedList.start is now value-fuzzed. column.width: the "50% churn" counterexample rested on a false premise — the canonical editor (editor-ext column.ts) stores width as a unitless flex-grow NUMBER (parseFloat, `flex:${width}`), never a "%" string, and docmost-schema.ts is a vendored mirror that MUST match it. The original parseFloat was correct parity and already byte-stable for numeric widths. Reverted the mirror to parseFloat, deleted the fabricated counterexample fixture, and now value-fuzz column.width as a number (real type). No src behaviour change for column.width — parity with editor-ext preserved. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/lib/docmost-schema.ts | 2 + .../src/lib/markdown-converter.ts | 22 ++++-- .../columns-column-width-percent.json | 16 ----- .../test/generative/attr-arbitraries.ts | 15 ++-- .../test/generative/counterexamples.test.ts | 68 ++++++------------- .../flat-roundtrip.property.test.ts | 8 +-- .../test/markdown-converter-gaps.test.ts | 7 +- .../test/markdown-converter-golden.test.ts | 12 ++-- .../markdown-converter-html-marks.test.ts | 14 ++-- 9 files changed, 68 insertions(+), 96 deletions(-) delete mode 100644 packages/prosemirror-markdown/test/fixtures/counterexamples/columns-column-width-percent.json diff --git a/packages/prosemirror-markdown/src/lib/docmost-schema.ts b/packages/prosemirror-markdown/src/lib/docmost-schema.ts index e27994e3..a1f4d734 100644 --- a/packages/prosemirror-markdown/src/lib/docmost-schema.ts +++ b/packages/prosemirror-markdown/src/lib/docmost-schema.ts @@ -1006,6 +1006,8 @@ const Column = Node.create({ width: { default: null, parseHTML: (el: HTMLElement) => { + // Mirrors editor-ext (column.ts): width is a unitless flex-grow + // number, so parse it to a Number for parity with the canonical schema. const value = el.getAttribute("data-width"); return value ? parseFloat(value) : null; }, diff --git a/packages/prosemirror-markdown/src/lib/markdown-converter.ts b/packages/prosemirror-markdown/src/lib/markdown-converter.ts index 481c3691..27363617 100644 --- a/packages/prosemirror-markdown/src/lib/markdown-converter.ts +++ b/packages/prosemirror-markdown/src/lib/markdown-converter.ts @@ -635,12 +635,20 @@ export function convertProseMirrorToMarkdown( .map((item: any) => processListItem(item, "-")) .join("\n"); - case "orderedList": + case "orderedList": { + // Honor a non-1 `start`: CommonMark expresses it as the first marker + // ("5." starts the list at 5), and marked parses that back into + //
      . Emitting `${start + index}.` keeps a start=5 list as + // "5.","6.",… while a default (start=1) list stays "1.","2.",… See #351. + // `Number(...) || 1` coerces a numeric-or-absent start and guards against + // a stray non-numeric `start` string-concatenating into the marker. + const start = Number(node.attrs?.start) || 1; return nodeContent .map((item: any, index: number) => - processListItem(item, `${index + 1}.`), + processListItem(item, `${start + index}.`), ) .join("\n"); + } case "taskList": return nodeContent.map((item: any) => processTaskItem(item)).join("\n"); @@ -1401,10 +1409,16 @@ export function convertProseMirrorToMarkdown( return `
        ${children .map((li: any) => `
      • ${blockChildrenToHtml(li)}
        • `) .join("")}
      `; - case "orderedList": - return `
        ${children + case "orderedList": { + // Carry a non-1 `start` on the raw-HTML path (columns/spanned cells) via + // the
          attribute, which the tiptap parser reads back into + // attrs.start. A default (start=1) list stays a bare
            . See #351. + const start = block.attrs?.start ?? 1; + const startAttr = start > 1 ? ` start="${start}"` : ""; + return `${children .map((li: any) => `
          1. ${blockChildrenToHtml(li)}
            2. `) .join("")}
          `; + } case "codeBlock": { const lang = block.attrs?.language || ""; // The code itself is element TEXT content (between tags), so it diff --git a/packages/prosemirror-markdown/test/fixtures/counterexamples/columns-column-width-percent.json b/packages/prosemirror-markdown/test/fixtures/counterexamples/columns-column-width-percent.json deleted file mode 100644 index c7af7dce..00000000 --- a/packages/prosemirror-markdown/test/fixtures/counterexamples/columns-column-width-percent.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "_bug": "BUG #351: a `column` whose `width` is a percentage string (e.g. \"50%\") is NOT byte-stable across export->import->export (violates P2). The `column` schema's parseHTML does `parseFloat(getAttribute('data-width'))`, which silently drops the '%' unit and returns the NUMBER 50. So the first export emits data-width=\"50%\" but the re-import stores width=50, and the second export emits data-width=\"50\": md2 !== md1, a permanent GS-EDIT-REVERT churn (every git-sync pull rewrites the column width). The editor authors column widths as percentages, so this is a real data/round-trip defect. Fix belongs in src/lib/docmost-schema.ts column.width parseHTML (preserve the unit / keep the string), which is OUT OF SCOPE for this test-only PR and must be a separate, maintainer-approved change. This flat generator therefore keeps `column.width` frozen (never generates a non-default width).", - "doc": { - "type": "doc", - "content": [ - { - "type": "columns", - "attrs": { "layout": "two_equal", "widthMode": "normal" }, - "content": [ - { "type": "column", "attrs": { "width": "50%" }, "content": [{ "type": "paragraph", "content": [{ "type": "text", "text": "L" }] }] }, - { "type": "column", "attrs": { "width": "50%" }, "content": [{ "type": "paragraph", "content": [{ "type": "text", "text": "R" }] }] } - ] - } - ] - } -} diff --git a/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts b/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts index 9fa7ccf1..b0f88fd4 100644 --- a/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts +++ b/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts @@ -134,10 +134,10 @@ const OVERRIDES: Record = { 'heading.textAlign': { arb: str('center', 'right', 'justify') }, 'heading.indent': { frozen: true }, // ACCEPTED: no md representation // ── lists ──────────────────────────────────────────────────────────────── - // PINNED-BUG: markdown CAN express a non-1 start ("5."), but the converter - // renders "1." and drops it -> P1 loss. See counterexamples.test.ts - // (ordered-list-start.json). Frozen only until the maintainer rules accept-vs-fix. - 'orderedList.start': { always: true, frozen: true }, + // FIXED (#351): the converter now emits the start marker ("5." /
            ) + // and it round-trips, so the start number is value-fuzzed. See + // counterexamples.test.ts (ordered-list-start.json) for the regression pin. + 'orderedList.start': { always: true, arb: num(2, 3, 5) }, 'orderedList.type': { frozen: true }, // ACCEPTED: a/A/i markers not expressible in GFM 'taskItem.checked': { always: true, arb: fc.constant(true) }, // boolean, default false // ── codeBlock ──────────────────────────────────────────────────────────── @@ -194,9 +194,10 @@ const OVERRIDES: Record = { // widthMode round-trips via the `data-width-mode` attribute (verified P1+P2), // so it is fuzzed, not frozen. 'columns.widthMode': { always: true, arb: str('custom') }, - // PINNED-BUG: parseFloat import drops the `%` unit -> P2 churn. See - // counterexamples.test.ts (columns-column-width-percent.json). - 'column.width': { frozen: true }, + // column.width is a unitless flex-grow NUMBER (matches editor-ext column.ts); + // parseHTML does parseFloat, so String(50) === "50" both ways and a numeric + // width round-trips byte-stably. Value-fuzzed as a number. + 'column.width': { arb: num(25, 50, 75) }, // ── embed (schema keeps width/height NUMERIC, not string-coerced) ───────── 'embed.src': { noDefault: true, arb: urlArb, degen: '' }, 'embed.provider': { noDefault: true, arb: str('iframe', 'youtube', 'vimeo') }, diff --git a/packages/prosemirror-markdown/test/generative/counterexamples.test.ts b/packages/prosemirror-markdown/test/generative/counterexamples.test.ts index 4d6afd40..bcf967ec 100644 --- a/packages/prosemirror-markdown/test/generative/counterexamples.test.ts +++ b/packages/prosemirror-markdown/test/generative/counterexamples.test.ts @@ -7,18 +7,17 @@ import { markdownToProseMirror } from '../../src/lib/markdown-to-prosemirror.js' import { docsCanonicallyEqual } from '../../src/lib/canonicalize.js'; // --------------------------------------------------------------------------- -// #351 committed counterexamples — REAL round-trip bugs surfaced by the flat -// generative probing (attribute level). Each is pinned here as an `it.fails` -// (vitest passes ONLY WHILE the assertion still fails), so that the day the -// underlying src/ bug is fixed, the `it.fails` starts PASSING and vitest turns -// this test RED — forcing us to delete the counterexample and (per the epic -// guardrail) tighten the generator. A bare `it.fails` would ship silent -// corruption, so every case below carries a loud `// BUG #351:` explanation. +// #351 committed counterexample — a REAL round-trip bug surfaced by the flat +// generative probing (attribute level). The bug below is now FIXED in src/ +// (maintainer-approved), so this case is a permanent PASSING regression pin: +// the exact document that once lost data now round-trips cleanly, and the +// fixture stays in test/fixtures/counterexamples/ forever (per the epic +// guardrail) to guard against a re-regression. The case carries a loud +// `// BUG #351 (FIXED):` note recording the defect and its fix site. // -// These bugs are NOT worked around by weakening any property: the offending -// attribute is kept OUT of the P1/P2 generators (documented in -// attr-arbitraries.ts), and the exact failing document lives here as the -// regression pin. FIXING the bug is a separate, maintainer-approved src/ change. +// With the bug fixed, the offending attribute is now VALUE-FUZZED by the +// generator (see attr-arbitraries.ts: orderedList.start), not held out — this +// committed document is the minimal, human-readable pin. // --------------------------------------------------------------------------- const here = path.dirname(fileURLToPath(import.meta.url)); @@ -28,47 +27,20 @@ function loadDoc(file: string): any { return JSON.parse(readFileSync(path.join(fixtureDir, file), 'utf8')).doc; } -describe('#351 counterexamples (known round-trip bugs, pinned as it.fails)', () => { - // BUG #351: a `column` with a PERCENTAGE width ("50%") is not byte-stable. - // The column schema parses `data-width` with parseFloat, dropping the '%': - // md1 = '...data-width="50%"...' (first export) - // re-import stores width = 50 (number) - // md2 = '...data-width="50"...' (second export) => md2 !== md1 - // A permanent GS-EDIT-REVERT churn on every git-sync pull. The editor stores - // column widths as percentages, so this is a genuine defect. The fix is in - // src/lib/docmost-schema.ts (column.width parseHTML must preserve the unit) - // and is out of scope for this test-only PR. - it.fails('column percentage width is byte-stable (P2)', async () => { - const doc = loadDoc('columns-column-width-percent.json'); - const md1 = convertProseMirrorToMarkdown(doc); - const doc2 = await markdownToProseMirror(md1); - const md2 = convertProseMirrorToMarkdown(doc2); - // This assertion currently FAILS (md2 drops the '%'), which is exactly what - // `it.fails` expects. When the schema is fixed, it will PASS and flip this - // test red — our cue to remove the pin. - expect(md2).toBe(md1); - }); - - // BUG #351: an `orderedList` with a non-1 `start` loses its start number. - // CommonMark CAN express this ("5." starts the list at 5), but the converter - // always emits "1." and ignores `attrs.start` (markdown-converter.ts renders - // `${index + 1}.`; the
              HTML path also omits `start`): +describe('#351 counterexamples (round-trip bugs, now FIXED — permanent regression pins)', () => { + // BUG #351 (FIXED): an `orderedList` with a non-1 `start` lost its start + // number. CommonMark CAN express this ("5." starts the list at 5), but the + // converter always emitted "1." and ignored `attrs.start`: // doc.start = 5 -> md1 = "1. alpha" (start dropped on export) - // re-import stores start = 1 => docsCanonicallyEqual(rt, doc) === false - // This is a P1 (semantic round-trip) loss of the SAME class as column.width: - // representable in markdown, silently dropped by the converter. It is pinned - // here as the LOUD counterexample rather than being masked as an "accepted - // normalization" in the generator — per the epic guardrail, deciding - // accept-vs-fix for a markdown-representable loss is a MAINTAINER call, so this - // stays a visible known-bug until the maintainer rules on it. The fix would be - // in src/lib/markdown-converter.ts (emit the start number on the first item) - // and is out of scope for this test-only PR. - it.fails('ordered list start number is preserved (P1)', async () => { + // re-import stored start = 1 => docsCanonicallyEqual(rt, doc) === false + // A P1 (semantic round-trip) loss of the SAME class as column.width. FIXED in + // this PR in src/lib/markdown-converter.ts (the orderedList markdown path emits + // `${start + index}.`; the raw-HTML path emits `
                `). tiptap's + // StarterKit / marked parse the start back, so it round-trips. Permanent P1 pin. + it('ordered list start number is preserved (P1)', async () => { const doc = loadDoc('ordered-list-start.json'); const md1 = convertProseMirrorToMarkdown(doc); const doc2 = await markdownToProseMirror(md1); - // Currently FAILS: doc2.start === 1 while doc.start === 5. When the converter - // preserves `start`, this PASSES and flips the test red — remove the pin then. expect(docsCanonicallyEqual(doc2, doc)).toBe(true); }); }); diff --git a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts index 46621537..5be0631c 100644 --- a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts +++ b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts @@ -33,13 +33,11 @@ import { // (aspectRatio, size, caption, drawio/excalidraw/pdf/video/youtube w/h/align) // — round-trip candidates deferred to a later PR, not silently dropped; // - ACCEPTED limitations with no md representation (indent, callout.icon, -// orderedList.type, table spans/bg/colwidth); -// - PINNED bugs (column.width, orderedList.start) tracked in -// counterexamples.test.ts. +// orderedList.type, table spans/bg/colwidth). const ATTR_VALUE_FUZZ_ALLOWLIST = new Set([ 'attachment.attachmentId', 'attachment.mime', 'attachment.placeholder', 'attachment.size', 'audio.attachmentId', 'audio.placeholder', 'audio.size', - 'callout.icon', 'column.width', + 'callout.icon', 'drawio.align', 'drawio.alt', 'drawio.aspectRatio', 'drawio.attachmentId', 'drawio.height', 'drawio.size', 'drawio.title', 'drawio.width', 'embed.align', 'embed.height', 'embed.width', @@ -48,7 +46,7 @@ const ATTR_VALUE_FUZZ_ALLOWLIST = new Set([ 'heading.indent', 'image.aspectRatio', 'image.attachmentId', 'image.caption', 'image.placeholder', 'image.size', 'mention.anchorId', 'mention.creatorId', 'mention.slugId', - 'orderedList.start', 'orderedList.type', 'paragraph.indent', + 'orderedList.type', 'paragraph.indent', 'pdf.attachmentId', 'pdf.height', 'pdf.placeholder', 'pdf.size', 'pdf.width', 'tableCell.backgroundColor', 'tableCell.backgroundColorName', 'tableCell.colspan', 'tableCell.colwidth', 'tableCell.rowspan', diff --git a/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts b/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts index 4f864fea..c3bb1e2b 100644 --- a/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts +++ b/packages/prosemirror-markdown/test/markdown-converter-gaps.test.ts @@ -217,8 +217,9 @@ const colChildOf = (doc2: any) => doc2?.content?.[0]?.content?.[0]?.content?.[0]; describe('converter gap coverage — emission branches (specs 1–11)', () => { - // 1. orderedList renders index+1 and DROPS the start attribute. - it('orderedList start:5 restarts numbering at 1 (start attr ignored)', () => { + // 1. orderedList honors the start attribute (FIXED #351): markers count up + // from `start` ("5.","6.",…), which CommonMark round-trips. + it('orderedList start:5 numbers from 5 (start attr honored)', () => { const out = convertProseMirrorToMarkdown( doc({ type: 'orderedList', @@ -229,7 +230,7 @@ describe('converter gap coverage — emission branches (specs 1–11)', () => { ], }), ); - expect(out).toBe('1. a\n2. b'); + expect(out).toBe('5. a\n6. b'); }); // 2. An empty paragraph contributes an empty segment between two "\n\n" joins. diff --git a/packages/prosemirror-markdown/test/markdown-converter-golden.test.ts b/packages/prosemirror-markdown/test/markdown-converter-golden.test.ts index 86a00e25..0cecf0de 100644 --- a/packages/prosemirror-markdown/test/markdown-converter-golden.test.ts +++ b/packages/prosemirror-markdown/test/markdown-converter-golden.test.ts @@ -365,7 +365,7 @@ describe('media / attachment / container full-attribute golden coverage', () => ); }); - it('orderedList inside a column renders via blockToHtml as
                  (start attr DROPPED) with bold->strong, code->code', () => { + it('orderedList inside a column renders via blockToHtml as
                    (start attr PRESERVED) with bold->strong, code->code', () => { const out = c({ type: 'columns', attrs: { layout: 'two' }, @@ -391,13 +391,13 @@ describe('media / attachment / container full-attribute golden coverage', () => }, ], }); - // blockToHtml orderedList path emits a plain
                      with no start attribute, - // and inlineToHtml maps bold->strong, code->code. + // blockToHtml orderedList path emits
                        (FIXED #351), and + // inlineToHtml maps bold->strong, code->code. expect(out).toContain( - '

                        1. a

                        2. b

                        ', + '

                        1. a

                        2. b

                        ', ); - // The start:3 attr is NOT preserved in the HTML/column container path. - expect(out).not.toContain('start='); + // The start:3 attr IS preserved in the HTML/column container path. + expect(out).toContain('start="3"'); }); it('hardBreak inside a column renders as
                        via inlineToHtml (not the markdown two-space form)', () => { diff --git a/packages/prosemirror-markdown/test/markdown-converter-html-marks.test.ts b/packages/prosemirror-markdown/test/markdown-converter-html-marks.test.ts index 8c011d9c..770d0e19 100644 --- a/packages/prosemirror-markdown/test/markdown-converter-html-marks.test.ts +++ b/packages/prosemirror-markdown/test/markdown-converter-html-marks.test.ts @@ -178,12 +178,12 @@ describe('blockToHtml: heading / codeBlock(lang & no-lang) / bulletList inside c // A colspan>1 cell forces the WHOLE table to the raw- HTML fallback // (markdown-converter.ts ~287-331). renderHtmlCell emits colspan + align attrs // (312-316) and renders each block child via blockToHtml. An orderedList child -// hits the blockToHtml orderedList branch (726-729), which emits -//

                        1. ..

                          2. ..
                        — the schema's `start` attr is NOT emitted by -// this HTML
                          branch. +// hits the blockToHtml orderedList branch, which emits +//

                          1. ..

                            2. ..
                          — the schema's non-1 `start` attr IS +// emitted by this HTML
                            branch (FIXED #351). // --------------------------------------------------------------------------- describe('spanned table: renderHtmlCell colspan/align + orderedList block child', () => { - it('renders the colspan/align cell with an
                              (start attr is dropped)', () => { + it('renders the colspan/align cell with an
                                (start attr preserved)', () => { const out = convertProseMirrorToMarkdown( doc({ type: 'table', @@ -213,11 +213,11 @@ describe('spanned table: renderHtmlCell colspan/align + orderedList block child' expect(out).toBe( '
                        ' + '' + '
                        ' + - '

                        1. one

                        2. two

                        ' + + '

                        1. one

                        2. two

                        ' + '
                        ', ); - // The HTML
                          branch does not propagate the ProseMirror `start` attribute. - expect(out).not.toContain('start'); + // The HTML
                            branch propagates the ProseMirror `start` attribute. + expect(out).toContain('start="3"'); }); }); -- 2.52.0 From 15fe998d3d123a3efcb44046f9aa4937113e5a68 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Mon, 6 Jul 2026 19:40:52 +0300 Subject: [PATCH 2/4] test(converter): value-fuzz the deferred media attributes (#351) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 4 of the #351 remainder — implement value-fuzz for the media dimension/family attributes previously parked in the flat property suite's ATTR_VALUE_FUZZ_ALLOWLIST as "round-trip candidates deferred to a later PR": width/height/align/aspectRatio/size/caption/title/alt across image, video, youtube, pdf, drawio, excalidraw, embed. Each gets a non-default-value arbitrary in attr-arbitraries.ts and is removed from the allowlist; the P1 (semantic) + P2 (byte-stability) property gate now exercises them. All round-trip green — the comment-JSON serialization (stable key order, String()-stringified) carries every one faithfully, so no new pins or accepted-limitations were needed. embed.width/height are fuzzed as numeric STRINGS (not numbers): a non-default embed dimension round-trips through the comment JSON as a string (the numeric 800/600 default is still omitted/re-materialized), so authoring a number diverged under P1. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/generative/attr-arbitraries.ts | 61 ++++++++++++++++++- .../flat-roundtrip.property.test.ts | 26 ++++---- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts b/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts index b0f88fd4..c3c9a03d 100644 --- a/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts +++ b/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts @@ -120,6 +120,11 @@ interface AttrPolicy { const num = (...xs: number[]) => fc.constantFrom(...xs); const str = (...xs: string[]) => fc.constantFrom(...xs); const widthStr = str('120', '320', '640'); +// Media `aspectRatio`/`size` are stringified numerics too (converter emits +// String(value)); the schema parseHTML reads them back as strings. Fuzz as +// plausible numeric strings so they round-trip byte-stably like widthStr. +const aspectRatioStr = str('1.5', '0.75', '1'); +const sizeStr = str('320', '640'); // The documented override table, keyed `type.attr`. Every entry is grounded in // the empirical converter probe (see flat-roundtrip.property.test.ts header). @@ -149,16 +154,57 @@ const OVERRIDES: Record = { 'image.title': { arb: letterPhraseArb }, 'image.width': { arb: widthStr, degen: '' }, 'image.height': { arb: widthStr, degen: '' }, + // #351 (this PR): media sizing/family attrs ride in the `` + // comment JSON via String(value); parseHTML reads them back as strings, so a + // numeric string round-trips byte-stably (mirrors image.width). + 'image.size': { arb: sizeStr, degen: '' }, + 'image.aspectRatio': { arb: aspectRatioStr }, + // caption is text carried verbatim in the comment JSON (mirrors image.alt). + 'image.caption': { arb: letterPhraseArb, degen: '' }, 'video.src': { noDefault: true, arb: urlArb, degen: '' }, 'video.alt': { arb: letterPhraseArb }, 'video.width': { arb: widthStr }, 'video.height': { arb: widthStr }, + // #351 (this PR): video sizing/align ride in the `` comment. + 'video.size': { arb: sizeStr }, + 'video.aspectRatio': { arb: aspectRatioStr }, + // align default "center" is dropped on export; fuzz non-center only. + 'video.align': { arb: str('left', 'right') }, 'audio.src': { noDefault: true, arb: urlArb, degen: '' }, 'youtube.src': { noDefault: true, arb: urlArb }, + // #351 (this PR): youtube width/height/align ride in the `` + // comment JSON (String()-coerced dimensions, non-center align only). + 'youtube.width': { arb: widthStr }, + 'youtube.height': { arb: widthStr }, + 'youtube.align': { arb: str('left', 'right') }, 'pdf.src': { noDefault: true, arb: urlArb }, 'pdf.name': { arb: phraseArb }, + // #351 (this PR): pdf size/width/height ride in the `` comment + // (String()-coerced dimensions, read back as strings). + 'pdf.size': { arb: sizeStr }, + 'pdf.width': { arb: widthStr }, + 'pdf.height': { arb: widthStr }, 'drawio.src': { noDefault: true, arb: urlArb }, + // #351 (this PR): drawio family attrs ride in the `` comment. + // Dimensions/size/aspectRatio are String()-coerced numeric strings; title/alt + // are text carried verbatim; align fuzzed non-center only. + 'drawio.width': { arb: widthStr }, + 'drawio.height': { arb: widthStr }, + 'drawio.size': { arb: sizeStr }, + 'drawio.aspectRatio': { arb: aspectRatioStr }, + 'drawio.align': { arb: str('left', 'right') }, + 'drawio.title': { arb: letterPhraseArb }, + 'drawio.alt': { arb: letterPhraseArb }, 'excalidraw.src': { noDefault: true, arb: urlArb }, + // #351 (this PR): excalidraw family attrs ride in the `` + // comment (same shape as the drawio family above). + 'excalidraw.width': { arb: widthStr }, + 'excalidraw.height': { arb: widthStr }, + 'excalidraw.size': { arb: sizeStr }, + 'excalidraw.aspectRatio': { arb: aspectRatioStr }, + 'excalidraw.align': { arb: str('left', 'right') }, + 'excalidraw.title': { arb: letterPhraseArb }, + 'excalidraw.alt': { arb: letterPhraseArb }, 'attachment.url': { noDefault: true, arb: urlArb }, 'attachment.name': { arb: phraseArb }, // ── callout / status ───────────────────────────────────────────────────── @@ -201,8 +247,19 @@ const OVERRIDES: Record = { // ── embed (schema keeps width/height NUMERIC, not string-coerced) ───────── 'embed.src': { noDefault: true, arb: urlArb, degen: '' }, 'embed.provider': { noDefault: true, arb: str('iframe', 'youtube', 'vimeo') }, - 'embed.width': { always: true, frozen: true }, - 'embed.height': { always: true, frozen: true }, + // #351 (this PR): the embed schema defaults width/height to the NUMBERS 800/600 + // and the converter only emits them when they differ. But the value round-trips + // as a STRING: export stringifies into the comment JSON (String(width)) and the + // import path (embedToHtml -> data-width -> embed parseHTML) reads it back as a + // string, so an authored NUMBER 400 would diverge under P1 (400 vs "400"). Fuzz + // as numeric STRINGS avoiding "800"/"600" so they round-trip byte-stably. The + // 800/600 numeric default state still round-trips (omitted on export, re- + // materialized as the numeric default). `always` stays because these are + // materialized on import but absent from canonicalize's KNOWN_DEFAULTS. + 'embed.width': { always: true, arb: str('400', '1000', '1200') }, + 'embed.height': { always: true, arb: str('300', '500', '900') }, + // align default "center" is dropped on export; fuzz non-center only. + 'embed.align': { arb: str('left', 'right') }, // ── subpages / math / htmlEmbed ────────────────────────────────────────── 'subpages.recursive': { always: true, arb: fc.constant(true) }, // boolean, default false 'mathBlock.text': { noDefault: true, arb: str('x^2', 'a < b', '\\frac{1}{2}'), degen: '' }, diff --git a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts index 5be0631c..dfdd2b8c 100644 --- a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts +++ b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts @@ -28,32 +28,32 @@ import { // a NEW attribute (or a newly-frozen one) that lands in this bucket flips the // snapshot test red and forces a reviewer to classify it. Each belongs to one of: // - internal/opaque ids & placeholders (attachmentId, slugId, placeholder, -// creatorId, anchorId) — no meaningful non-default to assert; -// - dimensions/among the media family with no standalone md form here -// (aspectRatio, size, caption, drawio/excalidraw/pdf/video/youtube w/h/align) -// — round-trip candidates deferred to a later PR, not silently dropped; +// creatorId, anchorId, mime) — no meaningful non-default to assert. These stay +// frozen: their value is an opaque token carried verbatim, not a round-trip +// shape worth fuzzing; // - ACCEPTED limitations with no md representation (indent, callout.icon, // orderedList.type, table spans/bg/colwidth). +// The media dimension/family attrs (image/video/youtube/drawio/excalidraw/pdf/ +// embed width/height/align/size/aspectRatio/caption/title/alt) that were +// previously "deferred to a later PR" are IMPLEMENTED (value-fuzzed) in THIS PR +// via the OVERRIDES table in attr-arbitraries.ts — they ride in the discriminator +// comment JSON and round-trip byte-stably, so they are no longer allowlisted. const ATTR_VALUE_FUZZ_ALLOWLIST = new Set([ 'attachment.attachmentId', 'attachment.mime', 'attachment.placeholder', 'attachment.size', 'audio.attachmentId', 'audio.placeholder', 'audio.size', 'callout.icon', - 'drawio.align', 'drawio.alt', 'drawio.aspectRatio', 'drawio.attachmentId', - 'drawio.height', 'drawio.size', 'drawio.title', 'drawio.width', - 'embed.align', 'embed.height', 'embed.width', - 'excalidraw.align', 'excalidraw.alt', 'excalidraw.aspectRatio', 'excalidraw.attachmentId', - 'excalidraw.height', 'excalidraw.size', 'excalidraw.title', 'excalidraw.width', + 'drawio.attachmentId', + 'excalidraw.attachmentId', 'heading.indent', - 'image.aspectRatio', 'image.attachmentId', 'image.caption', 'image.placeholder', 'image.size', + 'image.attachmentId', 'image.placeholder', 'mention.anchorId', 'mention.creatorId', 'mention.slugId', 'orderedList.type', 'paragraph.indent', - 'pdf.attachmentId', 'pdf.height', 'pdf.placeholder', 'pdf.size', 'pdf.width', + 'pdf.attachmentId', 'pdf.placeholder', 'tableCell.backgroundColor', 'tableCell.backgroundColorName', 'tableCell.colspan', 'tableCell.colwidth', 'tableCell.rowspan', 'tableHeader.backgroundColor', 'tableHeader.backgroundColorName', 'tableHeader.colspan', 'tableHeader.colwidth', 'tableHeader.rowspan', - 'video.align', 'video.aspectRatio', 'video.attachmentId', 'video.placeholder', 'video.size', - 'youtube.align', 'youtube.height', 'youtube.width', + 'video.attachmentId', 'video.placeholder', ]); // ── MARK attribute-value coverage ─────────────────────────────────────────── -- 2.52.0 From ce9f1a89801b2f7df4ea02689d82e8d48b02c4c5 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Mon, 6 Jul 2026 19:51:44 +0300 Subject: [PATCH 3/4] test(converter): nightly property cron + env knobs + counterexample README (#351) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Items 1 & 2 of the #351 remainder. Item 1 — the flat/nested generative property tests now read SEED and NUM_RUNS from PROPERTY_SEED / PROPERTY_NUM_RUNS (via an envInt helper that honors an explicit 0 and falls back to the current defaults — 20250705/300 flat, /100 nested — on unset/empty/non-numeric). New .github/workflows/nightly-property.yml runs the generative suite daily (and on workflow_dispatch) with a random seed and NUM_RUNS≈5000; on failure it files a Gitea issue containing fast-check's shrunk counterexample (jq-escaped, dedup'd by title). No build step — the suite imports the converter from src/, so a tsc error can't masquerade as a property failure. Item 2 — new packages/prosemirror-markdown/README.md documents the counterexample process (surface -> shrink -> permanent fixture in test/fixtures/counterexamples/ + counterexamples.test.ts -> fix the converter, never weaken a property; maintainer-approved ACCEPTED/allowlist entries carry a reason) plus the two golden layers, the coverage allowlist, and how to run with the env knobs. Linked from AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/nightly-property.yml | 122 ++++++++++++++++++ AGENTS.md | 2 +- packages/prosemirror-markdown/README.md | 105 +++++++++++++++ .../flat-roundtrip.property.test.ts | 22 +++- .../nested-roundtrip.property.test.ts | 12 +- 5 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 .github/workflows/nightly-property.yml create mode 100644 packages/prosemirror-markdown/README.md diff --git a/.github/workflows/nightly-property.yml b/.github/workflows/nightly-property.yml new file mode 100644 index 00000000..32ed103b --- /dev/null +++ b/.github/workflows/nightly-property.yml @@ -0,0 +1,122 @@ +name: Nightly property fuzz + +# The daily heavy property run for the ProseMirror<->Markdown converter +# (packages/prosemirror-markdown). The PR/CI test run keeps NUM_RUNS modest to +# stay under budget; this cron cranks it up (~5000) with a RANDOM seed to hunt +# for deeper round-trip counterexamples than a fixed-seed PR run can reach. +# +# Counterexample -> fixture workflow: when a run fails, fast-check prints the +# SHRUNK minimal counterexample plus the reproducing seed. This job files a +# Gitea issue containing that seed + counterexample. A human then commits the +# shrunk doc as a PERMANENT fixture under +# packages/prosemirror-markdown/test/fixtures/counterexamples/ with a case in +# counterexamples.test.ts, and FIXES the converter (never weakens a property to +# hide the bug). See packages/prosemirror-markdown/README.md. + +on: + schedule: + # 03:00 UTC daily. + - cron: '0 3 * * *' + workflow_dispatch: + inputs: + num_runs: + description: 'fast-check runs per property' + required: false + default: '5000' + seed: + description: 'fast-check seed (empty = random)' + required: false + default: '' + +permissions: + contents: read + issues: write + +jobs: + property-fuzz: + runs-on: ubuntu-latest + timeout-minutes: 60 + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up pnpm + uses: pnpm/action-setup@v4 + + - name: Set up Node + uses: actions/setup-node@v4 + with: + node-version: 22 + cache: pnpm + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + # No build step: the generative suite imports the converter from src/ + # directly (e.g. `from '../../src/lib/markdown-converter.js'`), so it runs + # against source without the package's build/. Skipping the build also + # keeps a tsc build error from masquerading as a property-test failure and + # filing a bogus counterexample issue. + - name: Resolve seed and run count + id: params + run: | + set -euo pipefail + SEED="${{ inputs.seed }}" + # Empty seed (cron, or a dispatch that left it blank) -> random. Combine + # two RANDOMs so the seed spans more than RANDOM's 0..32767 range. + [ -z "$SEED" ] && SEED=$(( (RANDOM << 15) | RANDOM )) + NUM_RUNS="${{ inputs.num_runs || '5000' }}" + echo "seed=$SEED" >> "$GITHUB_OUTPUT" + echo "num_runs=$NUM_RUNS" >> "$GITHUB_OUTPUT" + echo "Property fuzz: PROPERTY_SEED=$SEED PROPERTY_NUM_RUNS=$NUM_RUNS" + + - name: Run generative property suite + env: + PROPERTY_SEED: ${{ steps.params.outputs.seed }} + PROPERTY_NUM_RUNS: ${{ steps.params.outputs.num_runs }} + run: | + set -o pipefail + pnpm --filter @docmost/prosemirror-markdown exec \ + vitest run test/generative/ 2>&1 | tee property-output.txt + + # On failure fast-check has already printed the shrunk counterexample and + # its reproducing seed into property-output.txt. File a Gitea issue with + # the seed, run count and the tail of that output so a human can turn the + # counterexample into a permanent fixture (see the header comment). + - name: File issue on failure + if: failure() + env: + SEED: ${{ steps.params.outputs.seed }} + NUM_RUNS: ${{ steps.params.outputs.num_runs }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set -uo pipefail + TITLE="Nightly property test failure (seed=${SEED})" + + # Best-effort dedup: skip if an open issue with the shared title prefix + # already exists. A failure of this check must NOT block issue creation. + EXISTING="" + if EXISTING=$(curl -sS \ + -H "Authorization: token ${GITHUB_TOKEN}" \ + "${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/issues?state=open&limit=100"); then + if printf '%s' "$EXISTING" \ + | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{let a;try{a=JSON.parse(s)}catch{process.exit(1)}if(!Array.isArray(a))process.exit(1);process.exit(a.some(i=>typeof i.title==="string"&&i.title.startsWith("Nightly property test failure"))?0:1)})'; then + echo "An open 'Nightly property test failure' issue already exists — skipping creation." + exit 0 + fi + fi + + # Build the JSON body with the test output SAFELY escaped (never hand- + # interpolate the counterexample into JSON). + BODY_TEXT=$(printf 'The nightly property fuzz run failed.\n\n- seed: `%s`\n- NUM_RUNS: `%s`\n- run: %s\n\nReproduce locally:\n\n```\nPROPERTY_SEED=%s PROPERTY_NUM_RUNS=%s pnpm --filter @docmost/prosemirror-markdown exec vitest run test/generative/\n```\n\nfast-check shrinks the failure to a minimal counterexample. Commit it as a permanent fixture under `packages/prosemirror-markdown/test/fixtures/counterexamples/` + a case in `counterexamples.test.ts`, then FIX the converter (do not weaken a property). See `packages/prosemirror-markdown/README.md`.\n\nTail of the test output (contains the shrunk counterexample):\n\n```\n%s\n```\n' \ + "$SEED" "$NUM_RUNS" "$RUN_URL" "$SEED" "$NUM_RUNS" "$(tail -n 120 property-output.txt)") + + jq -n --arg title "$TITLE" --arg body "$BODY_TEXT" \ + '{title: $title, body: $body}' > payload.json + + curl -sS -X POST \ + "${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/issues" \ + -H "Authorization: token ${GITHUB_TOKEN}" \ + -H 'Content-Type: application/json' \ + -d @payload.json diff --git a/AGENTS.md b/AGENTS.md index 30c542ba..0e3a0a76 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -311,7 +311,7 @@ The API server is a Fastify app with a global `/api` prefix (`main.ts` excludes ### Client structure Vite SPA. Code is organized by feature under `apps/client/src/features/*` (mirrors the server domains: `page`, `space`, `comment`, `ai-chat`, `editor`, …). Conventions: - **TanStack Query** for server state (one `queries/` file per feature), **Jotai** atoms for local/shared UI state, **Mantine 8** + CSS modules (`*.module.css`) + `postcss-preset-mantine` for UI. -- The editor is Tiptap; shared node/mark extensions live in `packages/editor-ext` and are imported by **both the client and the server** (collaboration, schema, `canonicalizeFootnotes`) — editor schema changes often need to be made in `editor-ext`, not just the client. Server-side markdown import/export no longer lives in `editor-ext`: it goes through the canonical converter (#345, see below). The ProseMirror↔Markdown converter and its Docmost schema mirror now live in a SINGLE package, `@docmost/prosemirror-markdown` (#293), consumed by `mcp`, `git-sync`, and `apps/server` (#345) — do NOT reintroduce a per-package copy. `editor-ext` is the upstream source of the Tiptap schema; the package's `docmost-schema.ts` mirrors it and a serializer-contract test (`packages/prosemirror-markdown/test/serializer-contract.test.ts`) guards the boundary (every schema node must have a converter case), so a drift surfaces as a failing test rather than silent divergence. +- The editor is Tiptap; shared node/mark extensions live in `packages/editor-ext` and are imported by **both the client and the server** (collaboration, schema, `canonicalizeFootnotes`) — editor schema changes often need to be made in `editor-ext`, not just the client. Server-side markdown import/export no longer lives in `editor-ext`: it goes through the canonical converter (#345, see below). The ProseMirror↔Markdown converter and its Docmost schema mirror now live in a SINGLE package, `@docmost/prosemirror-markdown` (#293), consumed by `mcp`, `git-sync`, and `apps/server` (#345) — do NOT reintroduce a per-package copy. `editor-ext` is the upstream source of the Tiptap schema; the package's `docmost-schema.ts` mirrors it and a serializer-contract test (`packages/prosemirror-markdown/test/serializer-contract.test.ts`) guards the boundary (every schema node must have a converter case), so a drift surfaces as a failing test rather than silent divergence. For the converter's property-testing and counterexample→fixture process (P1–P4 invariants, the `PROPERTY_SEED`/`PROPERTY_NUM_RUNS` knobs, and the nightly fuzz workflow), see `packages/prosemirror-markdown/README.md`. - API access goes through `apps/client/src/lib/api-client.ts` (axios). The `@` alias maps to `apps/client/src`. - Runtime config is injected at build time by `vite.config.ts` via `define` (`APP_URL`, `COLLAB_URL`, `APP_VERSION`, …) — these come from the root `.env`, not from `import.meta.env`. diff --git a/packages/prosemirror-markdown/README.md b/packages/prosemirror-markdown/README.md new file mode 100644 index 00000000..0c60d7d6 --- /dev/null +++ b/packages/prosemirror-markdown/README.md @@ -0,0 +1,105 @@ +# @docmost/prosemirror-markdown + +The single, canonical **ProseMirror ↔ Markdown converter** plus the Docmost +schema mirror (#293/#345). Headless and framework-free: no React, no browser +runtime. There is exactly ONE copy of this converter in the repo, consumed by: + +- `packages/mcp` (the MCP server), +- `packages/git-sync` (two-way Git sync), +- `apps/server` (server-side markdown import/export, #345). + +`src/lib/docmost-schema.ts` **mirrors** the upstream Tiptap schema that lives in +`packages/editor-ext`. The mirror is not free-floating: `serializer-contract.test.ts` +guards the boundary — every schema node must have a converter case, so a drift +between `editor-ext` and this package surfaces as a failing test rather than a +silent divergence. + +## Why byte-stability matters + +Git sync exports a page to markdown, and re-imports it on the next pull. If +`export → import → export` is not **byte-stable**, every pull rewrites files +that nobody edited, and the user's history churns with phantom diffs. So the +converter is held to more than "it roughly round-trips": the second export pass +must be a byte-for-byte fixpoint. That is what the property suite below proves. + +## Two golden layers (do not mix them) + +1. **Corpus fixtures** — `test/fixtures/corpus/`. A fixed, hand-curated set of + representative documents (headings, marks, lists, tables, diagrams, columns, + details, mentions, …). These are the readable, deterministic "known-good" + snapshots. Edit them deliberately. + +2. **Generative property suite** — `test/generative/`. fast-check draws random + documents and asserts invariants over them. Two entry points: + - `flat-roundtrip.property.test.ts` — flat documents, attribute-level fuzzing. + - `nested-roundtrip.property.test.ts` — deeply nested structures. + + The invariants (**P1–P4**): + - **P1** — semantic round-trip: `mdToPm(pmToMd(doc))` is canonically equal to + `doc` (no data loss for the round-trip-supported space). + - **P2** — byte fixpoint: `pmToMd(mdToPm(pmToMd(doc))) === pmToMd(doc)` (the + first pass may normalize once; the second pass must be a fixpoint). + - **P3** — totality: neither converter throws; output is bounded. + - **P4** — parser fuzz totality: for ANY input string, `markdownToProseMirror` + does not throw and returns a schema-valid document. + + These invariants are kept **STRICT** — no `it.fails`, skip, or weakening. A + failure means the generator found a REAL converter bug. + +## The counterexample process (the DoD) + +This is the point of the generative layer. When a property run diverges: + +1. **A property run surfaces a divergence** (locally, in CI, or in the nightly + cron — see below). +2. **fast-check shrinks it** to a minimal, human-readable counterexample and + prints the reproducing seed. +3. **Commit the shrunk doc as a permanent fixture** under + `test/fixtures/counterexamples/`, with a matching case in + `counterexamples.test.ts`. The fixture stays forever, as a regression pin. +4. **FIX the converter** so the counterexample round-trips. **Never weaken a + property to hide the bug.** +5. If — and only if — a maintainer decides a particular markdown-representable + loss is genuinely acceptable, it is recorded as an **explicit ACCEPTED / + allowlist entry with a written reason**, not by silently relaxing an + invariant. + +### Attribute-coverage allowlist + +`flat-roundtrip.property.test.ts` maintains `ATTR_VALUE_FUZZ_ALLOWLIST`. The +suite asserts that every attribute in the live schema is EITHER value-fuzzed by +the generator OR explicitly listed in this allowlist. This forces any newly +added node attribute to be consciously classified — you cannot add an attr and +leave it silently un-exercised; the coverage test fails until you either fuzz it +or record why it is held out. + +## Running + +```sh +# The full package suite (corpus + generative + contract tests): +pnpm --filter @docmost/prosemirror-markdown test +``` + +The generative suite honours two env knobs (invalid/empty → falls back to the +default): + +| Env var | Default (flat) | Default (nested) | Meaning | +| -------------------- | -------------- | ---------------- | -------------------------------- | +| `PROPERTY_SEED` | `20250705` | `20250705` | fast-check seed (reproducibility) | +| `PROPERTY_NUM_RUNS` | `300` | `100` | runs per property | + +```sh +# Reproduce a specific counterexample seed with a bigger budget: +PROPERTY_SEED=12345 PROPERTY_NUM_RUNS=5000 \ + pnpm --filter @docmost/prosemirror-markdown exec vitest run test/generative/ +``` + +### Nightly cron + +`.github/workflows/nightly-property.yml` runs the generative suite every night +with `PROPERTY_NUM_RUNS` cranked to ~5000 and a **random** seed, to reach deeper +counterexamples than a fixed-seed PR run can. On failure it files a Gitea issue +containing the reproducing seed, the run count, and the tail of the output (the +shrunk counterexample), which kicks off the counterexample → fixture process +above. It can also be triggered manually (`workflow_dispatch`) with custom +`num_runs` / `seed`. diff --git a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts index dfdd2b8c..08aa7ac2 100644 --- a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts +++ b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts @@ -111,13 +111,21 @@ vi.setConfig({ testTimeout: 30000 }); // Fixed seed so every failure is reproducible; fast-check also prints the // shrunk counterexample. numRuns starts modest to keep CI under budget — the -// issue's CI target is ~300-500 per property; the nightly / PR 3 will crank -// this up further. Each property runs over the UNION (fc.oneof) of all flat -// node generators, so the runs are shared across node types (one test per -// property keeps the jsdom import cost and memory bounded — a per-generator × -// per-property matrix is ~200 heavy tests that OOMs the worker). -const SEED = 20250705; -const NUM_RUNS = 300; +// issue's CI target is ~300-500 per property. Both are overridable via the +// PROPERTY_SEED / PROPERTY_NUM_RUNS env vars (an invalid/empty value → NaN → +// falls back to the default below): the nightly cron +// (.github/workflows/nightly-property.yml) cranks NUM_RUNS to ~5000 with a +// random seed to hunt for deeper counterexamples. Each property runs over the +// UNION (fc.oneof) of all flat node generators, so the runs are shared across +// node types (one test per property keeps the jsdom import cost and memory +// bounded — a per-generator × per-property matrix is ~200 heavy tests that +// OOMs the worker). +// An unset/empty/non-numeric value falls back to the default; an explicit 0 is +// honored (a valid fast-check seed) — `Number(x) || default` would swallow it. +const envInt = (v: string | undefined, dflt: number): number => + v !== undefined && v !== '' && Number.isFinite(Number(v)) ? Number(v) : dflt; +const SEED = envInt(process.env.PROPERTY_SEED, 20250705); +const NUM_RUNS = envInt(process.env.PROPERTY_NUM_RUNS, 300); const P1_GENERATORS = buildGenerators('p1'); const FUZZ_GENERATORS = buildGenerators('fuzz'); diff --git a/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts b/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts index a5c773c0..d22dbf77 100644 --- a/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts +++ b/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts @@ -33,12 +33,20 @@ vi.setConfig({ testTimeout: 60000 }); // failure prints the shrunk minimal counterexample for triage. // --------------------------------------------------------------------------- -const SEED = 20250705; +// Both are overridable via the PROPERTY_SEED / PROPERTY_NUM_RUNS env vars (an +// invalid/empty value → NaN → falls back to the default below); the nightly +// cron (.github/workflows/nightly-property.yml) cranks NUM_RUNS up with a +// random seed to hunt for deeper counterexamples. +// An unset/empty/non-numeric value falls back to the default; an explicit 0 is +// honored (a valid fast-check seed) — `Number(x) || default` would swallow it. +const envInt = (v: string | undefined, dflt: number): number => + v !== undefined && v !== '' && Number.isFinite(Number(v)) ? Number(v) : dflt; +const SEED = envInt(process.env.PROPERTY_SEED, 20250705); // The nested walk builds far heavier docs than the flat suite (each P1/P2 run // parses the emitted markdown through jsdom), so keep the run count moderate to // hold runtime and worker memory in budget while still exercising deep // structures. P4 (cheap string parsing) runs at a higher count below. -const NUM_RUNS = 100; +const NUM_RUNS = envInt(process.env.PROPERTY_NUM_RUNS, 100); const pmToMd = (doc: unknown): string => convertProseMirrorToMarkdown(doc); const mdToPm = (md: string): Promise => markdownToProseMirror(md); -- 2.52.0 From 35f2c06f42ec6096e405da65f372b23231bb3521 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Mon, 6 Jul 2026 20:32:29 +0300 Subject: [PATCH 4/4] =?UTF-8?q?test(converter):=20#391=20review=20round=20?= =?UTF-8?q?=E2=80=94=20orderedList=20start=20guard=20+=20nightly=20hardeni?= =?UTF-8?q?ng=20(#351)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DO-1 (regression): the orderedList start hardening `Number(x)||1` let a negative/fractional start through into the marker ("-3.", "2.5.") — marked can't tokenize it, so the whole list re-imported as a paragraph (structure corruption); start=0 churned. Both the markdown and raw-HTML export paths now use `Number.isInteger(raw) && raw > 1 ? raw : 1`, so any degenerate start collapses to the default "1." markers / bare
                              (list always valid). New ordered-list-start-normalization test pins {0,-3,2.5} → valid start=1 list, byte-stable; the round-trip fuzz keeps to integers >=2 (num 2,3,5,42). DO-2 (nightly was non-functional): NUM_RUNS=5000 OOM'd the worker and the crash was misreported as a "counterexample" issue whose prefix-dedup then locked out all future issues. Reworked to shard 8 fresh vitest processes (600 runs each, distinct seeds, --max-old-space-size) so deep fuzzing never OOMs; a failing shard's output is preserved, and issue creation discriminates a real fast-check counterexample from an infra/OOM failure (distinct titles + scoped dedup). The two issue steps use `always() &&` so they actually run on the failure path. DO-3: envInt extracted to test/generative/env-int.ts + unit-tested. DO-4: nightly dispatch inputs go through env: (no ${{ }} in run:). DO-5: attr-arbitraries.ts docblock synced (column.width/orderedList.start are fixed+fuzzed, not pinned it.fails). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/nightly-property.yml | 168 ++++++++++++++---- .../src/lib/markdown-converter.ts | 15 +- .../test/generative/attr-arbitraries.ts | 29 +-- .../test/generative/env-int.test.ts | 29 +++ .../test/generative/env-int.ts | 10 ++ .../flat-roundtrip.property.test.ts | 4 +- .../nested-roundtrip.property.test.ts | 4 +- .../ordered-list-start-normalization.test.ts | 75 ++++++++ 8 files changed, 282 insertions(+), 52 deletions(-) create mode 100644 packages/prosemirror-markdown/test/generative/env-int.test.ts create mode 100644 packages/prosemirror-markdown/test/generative/env-int.ts create mode 100644 packages/prosemirror-markdown/test/ordered-list-start-normalization.test.ts diff --git a/.github/workflows/nightly-property.yml b/.github/workflows/nightly-property.yml index 32ed103b..207972cc 100644 --- a/.github/workflows/nightly-property.yml +++ b/.github/workflows/nightly-property.yml @@ -2,14 +2,24 @@ name: Nightly property fuzz # The daily heavy property run for the ProseMirror<->Markdown converter # (packages/prosemirror-markdown). The PR/CI test run keeps NUM_RUNS modest to -# stay under budget; this cron cranks it up (~5000) with a RANDOM seed to hunt +# stay under budget; this cron cranks up total coverage with random seeds to hunt # for deeper round-trip counterexamples than a fixed-seed PR run can reach. # -# Counterexample -> fixture workflow: when a run fails, fast-check prints the -# SHRUNK minimal counterexample plus the reproducing seed. This job files a -# Gitea issue containing that seed + counterexample. A human then commits the -# shrunk doc as a PERMANENT fixture under -# packages/prosemirror-markdown/test/fixtures/counterexamples/ with a case in +# WHY SHARDING: a single mega-run (~10000 fast-check runs) OOMs the vitest worker +# (empirically ~1625 runs -> "JS heap out of memory", ~2GB) because heap +# accumulates across the whole property run in one process. Instead this job runs +# SHARDS fresh vitest processes, each a MODERATE per-shard count with a DISTINCT +# derived seed, so total coverage ~= SHARDS x PER_SHARD_NUM_RUNS across processes +# that never accumulate heap. On the first failing shard we stop and keep that +# shard's output for triage. +# +# Counterexample -> fixture workflow: when a shard fails, fast-check prints the +# SHRUNK minimal counterexample plus the reproducing seed. This job files a Gitea +# issue containing that seed + counterexample ONLY when the output actually holds +# a fast-check counterexample; an infra failure (OOM/tsc/install, no +# counterexample) is filed under a DISTINCT title so it can never poison the +# counterexample dedup. A human then commits the shrunk doc as a PERMANENT fixture +# under packages/prosemirror-markdown/test/fixtures/counterexamples/ with a case in # counterexamples.test.ts, and FIXES the converter (never weakens a property to # hide the bug). See packages/prosemirror-markdown/README.md. @@ -20,11 +30,11 @@ on: workflow_dispatch: inputs: num_runs: - description: 'fast-check runs per property' + description: 'fast-check runs PER SHARD (8 shards run in sequence)' required: false - default: '5000' + default: '600' seed: - description: 'fast-check seed (empty = random)' + description: 'base fast-check seed (empty = random); shard i uses base+i' required: false default: '' @@ -57,60 +67,152 @@ jobs: # against source without the package's build/. Skipping the build also # keeps a tsc build error from masquerading as a property-test failure and # filing a bogus counterexample issue. - - name: Resolve seed and run count + - name: Resolve base seed and per-shard run count id: params + # Dispatch inputs are read via env (NOT interpolated into the shell body) + # to avoid script injection through a crafted input value. + env: + SEED_INPUT: ${{ inputs.seed }} + NUM_RUNS_INPUT: ${{ inputs.num_runs }} run: | set -euo pipefail - SEED="${{ inputs.seed }}" + SEED="${SEED_INPUT:-}" # Empty seed (cron, or a dispatch that left it blank) -> random. Combine # two RANDOMs so the seed spans more than RANDOM's 0..32767 range. [ -z "$SEED" ] && SEED=$(( (RANDOM << 15) | RANDOM )) - NUM_RUNS="${{ inputs.num_runs || '5000' }}" + NUM_RUNS="${NUM_RUNS_INPUT:-}" + [ -z "$NUM_RUNS" ] && NUM_RUNS=600 echo "seed=$SEED" >> "$GITHUB_OUTPUT" echo "num_runs=$NUM_RUNS" >> "$GITHUB_OUTPUT" - echo "Property fuzz: PROPERTY_SEED=$SEED PROPERTY_NUM_RUNS=$NUM_RUNS" + echo "Sharded property fuzz: BASE_SEED=$SEED PER_SHARD_NUM_RUNS=$NUM_RUNS SHARDS=8" - - name: Run generative property suite + - name: Run generative property suite (sharded) + id: fuzz env: - PROPERTY_SEED: ${{ steps.params.outputs.seed }} - PROPERTY_NUM_RUNS: ${{ steps.params.outputs.num_runs }} + BASE_SEED: ${{ steps.params.outputs.seed }} + PER_SHARD_NUM_RUNS: ${{ steps.params.outputs.num_runs }} + SHARDS: '8' run: | - set -o pipefail - pnpm --filter @docmost/prosemirror-markdown exec \ - vitest run test/generative/ 2>&1 | tee property-output.txt + set -uo pipefail + # Give each fresh process headroom, but rely on SHARDING (not a big heap) + # to avoid OOM: a moderate per-shard count in a process that starts clean. + export NODE_OPTIONS=--max-old-space-size=4096 + : > property-output.txt + FAILED=0 + FAIL_SEED="" + i=0 + while [ "$i" -lt "$SHARDS" ]; do + SHARD_SEED=$(( BASE_SEED + i )) + echo "=== shard $((i + 1))/$SHARDS: PROPERTY_SEED=$SHARD_SEED PROPERTY_NUM_RUNS=$PER_SHARD_NUM_RUNS ===" + # tee OVERWRITES property-output.txt each shard; since we break on the + # first failure, the file ends up holding exactly the failing shard's + # output (which carries the shrunk counterexample + reproducing seed). + if PROPERTY_SEED="$SHARD_SEED" PROPERTY_NUM_RUNS="$PER_SHARD_NUM_RUNS" \ + pnpm --filter @docmost/prosemirror-markdown exec \ + vitest run test/generative/ 2>&1 | tee property-output.txt; then + echo "shard $((i + 1)) passed" + else + echo "shard $((i + 1)) FAILED (seed=$SHARD_SEED) — stopping; keeping its output" + FAILED=1 + FAIL_SEED="$SHARD_SEED" + break + fi + i=$(( i + 1 )) + done + echo "failed=$FAILED" >> "$GITHUB_OUTPUT" + echo "fail_seed=$FAIL_SEED" >> "$GITHUB_OUTPUT" + exit "$FAILED" - # On failure fast-check has already printed the shrunk counterexample and - # its reproducing seed into property-output.txt. File a Gitea issue with - # the seed, run count and the tail of that output so a human can turn the - # counterexample into a permanent fixture (see the header comment). - - name: File issue on failure - if: failure() + # A GENUINE counterexample: fast-check printed a shrunk minimal case and its + # reproducing seed into property-output.txt. File a dedup-guarded issue whose + # title prefix is UNIQUE to counterexamples, so an infra failure (handled by + # the next step under a different title) can never poison this dedup. + - name: File counterexample issue + # always() is REQUIRED: the fuzz step exits nonzero on a failing shard, + # so a bare `if:` (implicitly success() && ...) would skip this step + # exactly when it must run. always() lets it run on the failure path. + if: always() && steps.fuzz.outputs.failed == '1' env: - SEED: ${{ steps.params.outputs.seed }} + FAIL_SEED: ${{ steps.fuzz.outputs.fail_seed }} NUM_RUNS: ${{ steps.params.outputs.num_runs }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + TITLE_PREFIX: 'Nightly property counterexample' run: | set -uo pipefail - TITLE="Nightly property test failure (seed=${SEED})" + # Discriminate counterexample vs infra failure by the fast-check + # signature. No signature -> leave it to the infra-failure step. + if ! grep -Eq 'Property failed after|Counterexample' property-output.txt; then + echo "No fast-check counterexample signature — infra failure, handled by the next step." + exit 0 + fi + TITLE="${TITLE_PREFIX} (seed=${FAIL_SEED})" - # Best-effort dedup: skip if an open issue with the shared title prefix - # already exists. A failure of this check must NOT block issue creation. + # Best-effort dedup: skip if an open issue with the counterexample title + # prefix already exists. A failure of this check must NOT block creation. EXISTING="" if EXISTING=$(curl -sS \ -H "Authorization: token ${GITHUB_TOKEN}" \ "${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/issues?state=open&limit=100"); then if printf '%s' "$EXISTING" \ - | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{let a;try{a=JSON.parse(s)}catch{process.exit(1)}if(!Array.isArray(a))process.exit(1);process.exit(a.some(i=>typeof i.title==="string"&&i.title.startsWith("Nightly property test failure"))?0:1)})'; then - echo "An open 'Nightly property test failure' issue already exists — skipping creation." + | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{let a;try{a=JSON.parse(s)}catch{process.exit(1)}if(!Array.isArray(a))process.exit(1);const p=process.env.TITLE_PREFIX;process.exit(a.some(i=>typeof i.title==="string"&&i.title.startsWith(p))?0:1)})'; then + echo "An open '${TITLE_PREFIX}' issue already exists — skipping creation." exit 0 fi fi # Build the JSON body with the test output SAFELY escaped (never hand- # interpolate the counterexample into JSON). - BODY_TEXT=$(printf 'The nightly property fuzz run failed.\n\n- seed: `%s`\n- NUM_RUNS: `%s`\n- run: %s\n\nReproduce locally:\n\n```\nPROPERTY_SEED=%s PROPERTY_NUM_RUNS=%s pnpm --filter @docmost/prosemirror-markdown exec vitest run test/generative/\n```\n\nfast-check shrinks the failure to a minimal counterexample. Commit it as a permanent fixture under `packages/prosemirror-markdown/test/fixtures/counterexamples/` + a case in `counterexamples.test.ts`, then FIX the converter (do not weaken a property). See `packages/prosemirror-markdown/README.md`.\n\nTail of the test output (contains the shrunk counterexample):\n\n```\n%s\n```\n' \ - "$SEED" "$NUM_RUNS" "$RUN_URL" "$SEED" "$NUM_RUNS" "$(tail -n 120 property-output.txt)") + BODY_TEXT=$(printf 'A nightly property fuzz SHARD failed with a fast-check counterexample.\n\n- failing shard seed: `%s`\n- NUM_RUNS (per shard): `%s`\n- run: %s\n\nReproduce locally:\n\n```\nPROPERTY_SEED=%s PROPERTY_NUM_RUNS=%s pnpm --filter @docmost/prosemirror-markdown exec vitest run test/generative/\n```\n\nfast-check shrinks the failure to a minimal counterexample. Commit it as a permanent fixture under `packages/prosemirror-markdown/test/fixtures/counterexamples/` + a case in `counterexamples.test.ts`, then FIX the converter (do not weaken a property). See `packages/prosemirror-markdown/README.md`.\n\nTail of the test output (contains the shrunk counterexample):\n\n```\n%s\n```\n' \ + "$FAIL_SEED" "$NUM_RUNS" "$RUN_URL" "$FAIL_SEED" "$NUM_RUNS" "$(tail -n 120 property-output.txt)") + + jq -n --arg title "$TITLE" --arg body "$BODY_TEXT" \ + '{title: $title, body: $body}' > payload.json + + curl -sS -X POST \ + "${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/issues" \ + -H "Authorization: token ${GITHUB_TOKEN}" \ + -H 'Content-Type: application/json' \ + -d @payload.json + + # An INFRA failure (OOM, tsc, install) has NO counterexample signature. File + # it under a DISTINCT title so it is visible but keeps the counterexample + # dedup (above) uncontaminated — a real counterexample can still file even + # while an infra issue is open. + - name: File infra failure issue + # always() is REQUIRED: the fuzz step exits nonzero on a failing shard, + # so a bare `if:` (implicitly success() && ...) would skip this step + # exactly when it must run. always() lets it run on the failure path. + if: always() && steps.fuzz.outputs.failed == '1' + env: + FAIL_SEED: ${{ steps.fuzz.outputs.fail_seed }} + NUM_RUNS: ${{ steps.params.outputs.num_runs }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + TITLE_PREFIX: 'Nightly property run infra failure' + run: | + set -uo pipefail + # Only file when there is NO counterexample signature (else the + # counterexample step owns it). + if grep -Eq 'Property failed after|Counterexample' property-output.txt; then + echo "Counterexample present — owned by the counterexample step." + exit 0 + fi + TITLE="${TITLE_PREFIX} (seed=${FAIL_SEED})" + + EXISTING="" + if EXISTING=$(curl -sS \ + -H "Authorization: token ${GITHUB_TOKEN}" \ + "${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/issues?state=open&limit=100"); then + if printf '%s' "$EXISTING" \ + | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>{let a;try{a=JSON.parse(s)}catch{process.exit(1)}if(!Array.isArray(a))process.exit(1);const p=process.env.TITLE_PREFIX;process.exit(a.some(i=>typeof i.title==="string"&&i.title.startsWith(p))?0:1)})'; then + echo "An open '${TITLE_PREFIX}' issue already exists — skipping creation." + exit 0 + fi + fi + + BODY_TEXT=$(printf 'A nightly property fuzz SHARD failed WITHOUT a fast-check counterexample (infra failure: OOM / build / install). This is NOT a converter round-trip bug.\n\n- failing shard seed: `%s`\n- NUM_RUNS (per shard): `%s`\n- run: %s\n\nInvestigate the run log (memory, dependency install, or a tsc/import error). The nightly counterexample dedup is intentionally separate from this issue.\n\nTail of the test output:\n\n```\n%s\n```\n' \ + "$FAIL_SEED" "$NUM_RUNS" "$RUN_URL" "$(tail -n 120 property-output.txt)") jq -n --arg title "$TITLE" --arg body "$BODY_TEXT" \ '{title: $title, body: $body}' > payload.json diff --git a/packages/prosemirror-markdown/src/lib/markdown-converter.ts b/packages/prosemirror-markdown/src/lib/markdown-converter.ts index 27363617..8389d2b0 100644 --- a/packages/prosemirror-markdown/src/lib/markdown-converter.ts +++ b/packages/prosemirror-markdown/src/lib/markdown-converter.ts @@ -640,9 +640,12 @@ export function convertProseMirrorToMarkdown( // ("5." starts the list at 5), and marked parses that back into //
                                . Emitting `${start + index}.` keeps a start=5 list as // "5.","6.",… while a default (start=1) list stays "1.","2.",… See #351. - // `Number(...) || 1` coerces a numeric-or-absent start and guards against - // a stray non-numeric `start` string-concatenating into the marker. - const start = Number(node.attrs?.start) || 1; + // Only an INTEGER ≥ 2 is a valid explicit start; collapse everything else + // (absent, non-number, 0, negative, fractional) to 1. A negative/fractional + // start would otherwise emit an untokenizable marker (e.g. "-3.", "2.5.") + // that marked cannot parse, re-importing the whole list as a PARAGRAPH. + const raw = node.attrs?.start; + const start = Number.isInteger(raw) && (raw as number) > 1 ? (raw as number) : 1; return nodeContent .map((item: any, index: number) => processListItem(item, `${start + index}.`), @@ -1413,7 +1416,11 @@ export function convertProseMirrorToMarkdown( // Carry a non-1 `start` on the raw-HTML path (columns/spanned cells) via // the
                                  attribute, which the tiptap parser reads back into // attrs.start. A default (start=1) list stays a bare
                                    . See #351. - const start = block.attrs?.start ?? 1; + // Use the SAME integer-≥-2 guard as the markdown path: a fractional start + // would emit `start="2.5"` → parseInt→2 on import (path divergence), and a + // 0/negative start is not a valid explicit start either. + const raw = block.attrs?.start; + const start = Number.isInteger(raw) && (raw as number) > 1 ? (raw as number) : 1; const startAttr = start > 1 ? ` start="${start}"` : ""; return `${children .map((li: any) => `
                                  1. ${blockChildrenToHtml(li)}
                                    2. `) diff --git a/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts b/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts index c3c9a03d..f5cd05e3 100644 --- a/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts +++ b/packages/prosemirror-markdown/test/generative/attr-arbitraries.ts @@ -37,14 +37,19 @@ * tagged `// ACCEPTED:` inline. Freezing them is correct — there is * nothing to preserve in the target format. * - * (b) PINNED BUG — the attribute IS representable in markdown but the - * converter drops it anyway (a real defect). These are NOT silently - * frozen: each is captured as a LOUD `it.fails` counterexample in - * test/fixtures/counterexamples/ + counterexamples.test.ts, and the - * freeze here only keeps the P1/P2 union green until a MAINTAINER rules - * on accept-vs-fix (the epic guardrail reserves that call). These: - * `column.width` (parseFloat drops `%`), `orderedList.start` (non-1 - * start renders as `1.`). Tagged `// PINNED-BUG:` inline. + * (b) FIXED & VALUE-FUZZED — attributes that were once PINNED converter + * bugs (representable in markdown but dropped) and are now FIXED in + * src/, so they are value-fuzzed here at legal non-default values like + * any healthy attr. `orderedList.start` (the non-1 start once rendered + * as `1.`; the converter now emits the start marker / `
                                      `) + * and `column.width` (a unitless flex-grow number that round-trips via + * parseFloat) are both fuzzed in OVERRIDES below. The former held-out + * `it.fails` cases are gone; `ordered-list-start.json` + + * counterexamples.test.ts now stand as PASSING regression pins (per the + * epic guardrail, the minimal doc stays forever to guard re-regression). + * The #351 media-family sizing attrs (image/video/youtube/pdf/drawio/ + * excalidraw/embed width/height/size/aspectRatio) are likewise fuzzed + * now that they ride round-trip-safely in the per-node `` JSON. * * (c) DEFERRED-BUG — representable AND round-trips, frozen only because the * flat generator can't yet build a valid instance. Table @@ -64,8 +69,10 @@ * number re-parses as a string), EXCEPT `embed.width/height` which the * embed schema keeps numeric — handled per-attr. * - * Both PINNED-BUG attrs (`column.width` P2 churn, `orderedList.start` P1 loss) - * are captured as committed `it.fails` counterexamples — NOT hidden here. + * The two former PINNED-BUG attrs (`column.width` P2 churn, `orderedList.start` + * P1 loss) are now FIXED and value-fuzzed; `ordered-list-start.json` in + * counterexamples.test.ts is a permanent PASSING regression pin, not an + * `it.fails` hold-out. */ import fc from 'fast-check'; import { getSchema } from '@tiptap/core'; @@ -142,7 +149,7 @@ const OVERRIDES: Record = { // FIXED (#351): the converter now emits the start marker ("5." /
                                        ) // and it round-trips, so the start number is value-fuzzed. See // counterexamples.test.ts (ordered-list-start.json) for the regression pin. - 'orderedList.start': { always: true, arb: num(2, 3, 5) }, + 'orderedList.start': { always: true, arb: num(2, 3, 5, 42) }, 'orderedList.type': { frozen: true }, // ACCEPTED: a/A/i markers not expressible in GFM 'taskItem.checked': { always: true, arb: fc.constant(true) }, // boolean, default false // ── codeBlock ──────────────────────────────────────────────────────────── diff --git a/packages/prosemirror-markdown/test/generative/env-int.test.ts b/packages/prosemirror-markdown/test/generative/env-int.test.ts new file mode 100644 index 00000000..d23a45d9 --- /dev/null +++ b/packages/prosemirror-markdown/test/generative/env-int.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest'; +import { envInt } from './env-int.js'; + +// Unit coverage for the shared PROPERTY_SEED / PROPERTY_NUM_RUNS parser. The key +// contract is that an explicit "0" is honored (a valid fast-check seed) while +// empty/absent/non-numeric fall back to the default. +describe('envInt', () => { + it('honors an explicit "0" (does not fall back)', () => { + expect(envInt('0', 42)).toBe(0); + }); + it('falls back on empty string', () => { + expect(envInt('', 42)).toBe(42); + }); + it('falls back on undefined', () => { + expect(envInt(undefined, 42)).toBe(42); + }); + it('falls back on a non-numeric string', () => { + expect(envInt('abc', 42)).toBe(42); + }); + it('parses a plain integer string', () => { + expect(envInt('300', 42)).toBe(300); + }); + it('parses a negative integer', () => { + expect(envInt('-5', 42)).toBe(-5); + }); + it('parses a large integer', () => { + expect(envInt('1073741824', 42)).toBe(1073741824); + }); +}); diff --git a/packages/prosemirror-markdown/test/generative/env-int.ts b/packages/prosemirror-markdown/test/generative/env-int.ts new file mode 100644 index 00000000..9c7df021 --- /dev/null +++ b/packages/prosemirror-markdown/test/generative/env-int.ts @@ -0,0 +1,10 @@ +/** + * Parse an integer-ish environment variable with a default fallback. + * + * Used to read PROPERTY_SEED / PROPERTY_NUM_RUNS in the generative property + * suites. An unset/empty/non-numeric value falls back to `dflt`, but an explicit + * `"0"` is honored (a valid fast-check seed) — `Number(x) || dflt` would wrongly + * swallow 0. See flat-roundtrip.property.test.ts / nested-roundtrip.property.test.ts. + */ +export const envInt = (v: string | undefined, dflt: number): number => + v !== undefined && v !== '' && Number.isFinite(Number(v)) ? Number(v) : dflt; diff --git a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts index 08aa7ac2..87c6093a 100644 --- a/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts +++ b/packages/prosemirror-markdown/test/generative/flat-roundtrip.property.test.ts @@ -18,6 +18,7 @@ import { coveredTypes, KNOWN_UNCOVERED, } from './node-generators.js'; +import { envInt } from './env-int.js'; // ── Attribute-value coverage allowlist ────────────────────────────────────── // The node/mark completeness contract guarantees every TYPE is generated, but @@ -122,8 +123,7 @@ vi.setConfig({ testTimeout: 30000 }); // OOMs the worker). // An unset/empty/non-numeric value falls back to the default; an explicit 0 is // honored (a valid fast-check seed) — `Number(x) || default` would swallow it. -const envInt = (v: string | undefined, dflt: number): number => - v !== undefined && v !== '' && Number.isFinite(Number(v)) ? Number(v) : dflt; +// The parser is shared with the nested suite (env-int.ts) and unit-tested there. const SEED = envInt(process.env.PROPERTY_SEED, 20250705); const NUM_RUNS = envInt(process.env.PROPERTY_NUM_RUNS, 300); diff --git a/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts b/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts index d22dbf77..2a8a6768 100644 --- a/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts +++ b/packages/prosemirror-markdown/test/generative/nested-roundtrip.property.test.ts @@ -11,6 +11,7 @@ import { } from '../../src/lib/index.js'; import { firstDivergence } from '../roundtrip-helpers.js'; import { schema, docArb } from './doc-generator.js'; +import { envInt } from './env-int.js'; // Each run does a real convert + jsdom parse; give ample headroom so the suite // is deterministic under parallel worker load (matching the flat sibling suite). @@ -39,8 +40,7 @@ vi.setConfig({ testTimeout: 60000 }); // random seed to hunt for deeper counterexamples. // An unset/empty/non-numeric value falls back to the default; an explicit 0 is // honored (a valid fast-check seed) — `Number(x) || default` would swallow it. -const envInt = (v: string | undefined, dflt: number): number => - v !== undefined && v !== '' && Number.isFinite(Number(v)) ? Number(v) : dflt; +// The parser is shared with the flat suite (env-int.ts) and unit-tested there. const SEED = envInt(process.env.PROPERTY_SEED, 20250705); // The nested walk builds far heavier docs than the flat suite (each P1/P2 run // parses the emitted markdown through jsdom), so keep the run count moderate to diff --git a/packages/prosemirror-markdown/test/ordered-list-start-normalization.test.ts b/packages/prosemirror-markdown/test/ordered-list-start-normalization.test.ts new file mode 100644 index 00000000..03593afc --- /dev/null +++ b/packages/prosemirror-markdown/test/ordered-list-start-normalization.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from 'vitest'; +import { convertProseMirrorToMarkdown } from '../src/lib/markdown-converter.js'; +import { markdownToProseMirror } from '../src/lib/markdown-to-prosemirror.js'; + +// --------------------------------------------------------------------------- +// #351 regression — DEGENERATE orderedList `start` normalization (DO-1). +// +// Only an INTEGER >= 2 is a valid explicit start. A degenerate start (0, +// negative, fractional, non-number) MUST collapse to the default 1 on export: +// emitting the raw value would produce an untokenizable marker (e.g. "-3.", +// "2.5.") that `marked` cannot parse, causing the WHOLE orderedList (and its +// listItems) to re-import as a PARAGRAPH — structural corruption. This test +// pins that each degenerate start: +// (1) exports to "1."/"2." markers (default numbering), +// (2) re-imports as a VALID `orderedList` (not a paragraph) with `start` +// defaulting, structure preserved, +// (3) is byte-stable (md1 === md2). +// +// Mutation check: revert the converter guard to `Number(raw) || 1` and this +// test goes RED (a fractional/negative start leaks into the marker). +// --------------------------------------------------------------------------- + +const makeDoc = (start: unknown) => ({ + type: 'doc', + content: [ + { + type: 'orderedList', + attrs: { type: null, start }, + content: [ + { + type: 'listItem', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: 'alpha' }] }, + ], + }, + { + type: 'listItem', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: 'beta' }] }, + ], + }, + ], + }, + ], +}); + +describe('#351 orderedList degenerate start normalization', () => { + for (const start of [0, -3, 2.5]) { + it(`start=${start} exports as default "1."/"2." markers`, () => { + const md = convertProseMirrorToMarkdown(makeDoc(start)); + expect(md).toContain('1. alpha'); + expect(md).toContain('2. beta'); + // The degenerate value must NOT leak into any marker. + expect(md).not.toMatch(/-?\d*\.\d+\.\s/); // no fractional marker like "2.5." + expect(md).not.toContain('-3.'); + }); + + it(`start=${start} re-imports as a valid orderedList (not a paragraph)`, async () => { + const md = convertProseMirrorToMarkdown(makeDoc(start)); + const doc = (await markdownToProseMirror(md)) as any; + const top = doc.content?.[0]; + expect(top?.type).toBe('orderedList'); + // start defaults (never the degenerate value); tiptap materializes 1. + expect(top?.attrs?.start ?? 1).toBe(1); + expect(top?.content?.length).toBe(2); + }); + + it(`start=${start} is byte-stable across a re-export`, async () => { + const md1 = convertProseMirrorToMarkdown(makeDoc(start)); + const doc = await markdownToProseMirror(md1); + const md2 = convertProseMirrorToMarkdown(doc); + expect(md2).toBe(md1); + }); + } +}); -- 2.52.0