From 837defab6e2e9f8b14e9acfc7f8ded5a54498e69 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 12:55:30 +0800 Subject: [PATCH 01/11] fix(markdown): keep italic beside bold and strike beside CJK punctuation on save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The weekly soak's editing fuzz (#1407) found italic typed next to bold lost on save. Emphasis and strong were both written with `*`, and flush `*` delimiters merge into one run: `wordwor**wordword***# *`. micromark decides whether a run opens or closes from the characters around the WHOLE run, which neither node looked at, so the italic came back as literal asterisks (fuzz seeds 20260805, 5 and 6). Emphasis flush against a strong sibling is now written `_`, which never merges with `**`. Everywhere else it stays `*`, so existing documents keep their spelling. The strikethrough handler had the same class of fault from the other side: it tested ASCII punctuation only, while micromark counts every Unicode P and S character, so `文字~~。word~~` reparsed as literal tildes. All three attention delimiters now go through one module with one flanking model: micromark's character classes, upstream's encoding table, and character references that always cover a whole code point. --- .../__tests__/markEdgeWhitespace.test.ts | 4 +- src/utils/markdownPipeline/serializer.ts | 15 +- .../serializerAttention.test.ts | 112 +++++++++++ .../markdownPipeline/serializerAttention.ts | 190 ++++++++++++++++++ .../serializerStrikethrough.ts | 115 +---------- 5 files changed, 320 insertions(+), 116 deletions(-) create mode 100644 src/utils/markdownPipeline/serializerAttention.test.ts create mode 100644 src/utils/markdownPipeline/serializerAttention.ts diff --git a/src/utils/markdownPipeline/__tests__/markEdgeWhitespace.test.ts b/src/utils/markdownPipeline/__tests__/markEdgeWhitespace.test.ts index 138319c4c..e030eb065 100644 --- a/src/utils/markdownPipeline/__tests__/markEdgeWhitespace.test.ts +++ b/src/utils/markdownPipeline/__tests__/markEdgeWhitespace.test.ts @@ -70,8 +70,8 @@ it( // diagnosis that delimiter policy "must consider flanking context, not // only edge whitespace". // - // That is what `serializerStrikethrough.ts` now does (audit 20260906): the - // `delete` handler character-references the offending neighbour, the same + // That is what the `delete` handler in `serializerAttention.ts` does (audit + // 20260906): it character-references the offending neighbour, the same // remedy mdast-util-to-markdown already applies to emphasis and strong. // Flipped RED-to-GREEN deliberately, as the original note intended. const bold = schema.marks.bold.create(); diff --git a/src/utils/markdownPipeline/serializer.ts b/src/utils/markdownPipeline/serializer.ts index 58ccba313..f1eb503b2 100644 --- a/src/utils/markdownPipeline/serializer.ts +++ b/src/utils/markdownPipeline/serializer.ts @@ -5,7 +5,9 @@ * The serializer configuration determines VMark's canonical markdown style. * * Key decisions: - * - Bullet: `-` (not `*`), emphasis: `*`, strong: `**`, fence: backtick + * - Bullet: `-` (not `*`), emphasis: `*`, strong: `**`, fence: backtick. + * Emphasis flush against a `**` sibling is written `_` instead, because + * flush `*` runs merge (serializerAttention.ts) * - listItemIndent: "one" — minimizes diff noise compared to "tab" * - Custom handlers for image/link (serializerHandlers.ts): angle brackets * for URLs with spaces instead of percent-encoding, and autolink @@ -27,7 +29,8 @@ import { unified } from "unified"; import remarkStringify from "remark-stringify"; -import { handleDelete, repairSplitSurrogateEntities } from "./serializerStrikethrough"; +import { repairSplitSurrogateEntities } from "./serializerStrikethrough"; +import { handleDelete, handleEmphasis, handleStrong } from "./serializerAttention"; import remarkGfm from "remark-gfm"; import remarkMath from "remark-math"; import remarkFrontmatter from "remark-frontmatter"; @@ -70,9 +73,11 @@ function buildSerializer() { handlers: { image: handleImage, link: handleLink, - // `~~` obeys the same flanking rules as `*`, but the gfm - // strikethrough extension never adopted remark's neighbour-encoding - // fix — so `plain~~* word~~` was emitted as literal text on reparse. + // Attention delimiters share one flanking model, including the + // alternate `_` that keeps emphasis from merging into a neighbouring + // `**` (serializerAttention.ts). + emphasis: handleEmphasis, + strong: handleStrong, delete: handleDelete, ...tocToMarkdown.handlers, } as Record, diff --git a/src/utils/markdownPipeline/serializerAttention.test.ts b/src/utils/markdownPipeline/serializerAttention.test.ts new file mode 100644 index 000000000..b3b315ae4 --- /dev/null +++ b/src/utils/markdownPipeline/serializerAttention.test.ts @@ -0,0 +1,112 @@ +// @vitest-environment node +/** + * Attention delimiters (`*`, `**`, `~~`) must come back as the marks they were. + * + * Found by the weekly soak's editing fuzz (#1407) once its seed stopped being + * silently 0: italic typed next to bold was saved as a merged `***` run that no + * longer flanked, and the italic came back as literal asterisks in the author's + * text. The traces below are the fuzz's own minimized counterexamples, reduced + * to the document they produce. + */ +import { describe, expect, it } from "vitest"; +import type { Node as PMNode } from "@tiptap/pm/model"; +import { getProductionSchema } from "@/test/productionSchema"; +import { parseMarkdown, serializeMarkdown } from "./adapter"; + +const schema = getProductionSchema(); + +type MarkName = "bold" | "italic" | "strike"; +type Run = [text: string, marks: MarkName[]]; + +/** A one-paragraph document from `[text, marks]` runs. */ +function paragraph(runs: Run[]): PMNode { + return schema.node("doc", null, [ + schema.node( + "paragraph", + null, + runs.map(([text, marks]) => schema.text(text, marks.map((m) => schema.marks[m].create()))), + ), + ]); +} + +/** The document's text as `[text, sorted marks]` runs, adjacent equal runs merged. */ +function runsOf(doc: PMNode): Run[] { + const out: Run[] = []; + doc.descendants((node) => { + if (!node.isText) return; + const marks = node.marks.map((m) => m.type.name as MarkName).sort(); + const prev = out[out.length - 1]; + if (prev && prev[1].join() === marks.join()) prev[0] += node.text ?? ""; + else out.push([node.text ?? "", marks]); + }); + return out; +} + +function roundTrip(runs: Run[]): { markdown: string; runs: Run[] } { + const markdown = serializeMarkdown(schema, paragraph(runs)); + return { markdown, runs: runsOf(parseMarkdown(schema, markdown)) }; +} + +const sorted = (runs: Run[]): Run[] => runs.map(([t, m]) => [t, [...m].sort()]); + +describe("emphasis beside strong", () => { + // Seed 20260805: `wordwor**wordword***# *` — the italic merged into the + // bold's closing run, which cannot open before `#`, so it was lost. + it("keeps italic that directly follows bold (seed 20260805)", () => { + const runs: Run[] = [["wordwor", []], ["wordword", ["bold"]], ["# ", ["italic"]]]; + expect(roundTrip(runs).runs).toEqual(runs); + }); + + // Seed 5: `**עברית ***word*` — the merged run could not close the bold. + it("keeps bold that directly precedes italic (seed 5)", () => { + const runs: Run[] = [["עברית ", []], ["עברית ", ["bold"]], ["word", ["italic"]]]; + expect(roundTrip(runs).runs).toEqual(runs); + }); + + // Seed 6: `***wordb** ***b** word` — the italic's closer merged into the + // next bold's opener. + it("keeps italic that directly precedes bold (seed 6)", () => { + const runs: Run[] = [ + ["wordb", ["bold", "italic"]], + [" ", ["italic"]], + ["b", ["bold"]], + [" word", []], + ]; + expect(roundTrip(runs).runs).toEqual(sorted(runs)); + }); + + it.each<[string, Run[]]>([ + ["letters on the far side", [["a", ["bold"]], ["b", ["italic"]], ["c", []]]], + ["CJK on the far side", [["中文", []], ["粗体", ["bold"]], ["斜体", ["italic"]], ["中文", []]]], + ["punctuation inside", [["x", []], ["#", ["bold"]], [".", ["italic"]], ["y", []]]], + ["an emoji as the italic", [["a", ["bold"]], ["🙂", ["italic"]], ["b", []]]], + ["italic, bold, italic", [["a", ["italic"]], ["b", ["bold"]], ["c", ["italic"]]]], + ])("round-trips with %s", (_label, runs) => { + const back = roundTrip(runs); + expect(back.runs).toEqual(runs); + expect(back.markdown).not.toContain("�"); + }); + + // The alternate marker is a fix for one adjacency, not a new house style. + it("keeps `*` wherever it already round-trips", () => { + expect(roundTrip([["a ", []], ["b", ["italic"]], [" c", []]]).markdown).toBe("a *b* c\n"); + expect(roundTrip([["x", ["bold", "italic"]]]).markdown).toBe("***x***\n"); + expect(roundTrip([["a", ["bold"]], [" ", []], ["b", ["italic"]]]).markdown).toBe("**a** *b*\n"); + }); + + it("spells italic beside bold with `_`, which cannot merge into `**`", () => { + expect(roundTrip([["a", ["bold"]], ["b", ["italic"]]]).markdown).toBe("**a**_b_\n"); + }); +}); + +describe("strikethrough beside non-ASCII punctuation", () => { + // `文字~~。word~~` reparsed as literal tildes: the delimiter test counted only + // ASCII punctuation, while micromark counts every Unicode P and S character. + it.each<[string, Run[]]>([ + ["full stop opening the run", [["文字", []], ["。word", ["strike"]]]], + ["full stop closing the run", [["word。", ["strike"]], ["文字", []]]], + ["full-width parenthesis", [["文字", []], ["(注)", ["strike"]], ["文字", []]]], + ])("keeps the strike with a %s", (_label, runs) => { + expect(roundTrip(runs).runs).toEqual(runs); + }); +}); diff --git a/src/utils/markdownPipeline/serializerAttention.ts b/src/utils/markdownPipeline/serializerAttention.ts new file mode 100644 index 000000000..9d6c7242a --- /dev/null +++ b/src/utils/markdownPipeline/serializerAttention.ts @@ -0,0 +1,190 @@ +/** + * Attention delimiters — `*emphasis*`, `**strong**` and `~~strikethrough~~`. + * + * Purpose: emit every attention delimiter where VMark's parser (micromark) will + * read it back as the same mark. One module owns all three because they share + * one rule set: a run of `*`, `_` or `~` opens or closes depending only on the + * characters on either side of the RUN, so a decision made for one mark is + * wrong whenever a neighbour changes what those characters are. + * + * Key decisions: + * - Flanking is fixed the way mdast-util-to-markdown fixes it: a character + * beside a delimiter that stops it flanking is written as a character + * reference, which is punctuation to the parser and decodes back to the + * same text. The decision table (`encodeSides`) is upstream's `encodeInfo`. + * - Characters are classified exactly as micromark does (`classify`): Unicode + * P and S categories count as punctuation, per UTF-16 code unit. The old + * `delete` handler tested ASCII punctuation only, so `文字~~。word~~` came + * back as literal tildes. + * - Emphasis beside a `strong` sibling is written with `_`. Flush `*` + * delimiters merge into one run — `**a***b*` — whose flanking is decided by + * characters neither node looked at, and italic typed next to bold was lost + * on save (#1407 soak: seeds 20260805, 5, 6). `_` and `*` never merge. + * Everywhere else emphasis stays `*`, the house style; a parent/child edge + * such as `***x***` is fine as it is and keeps its spelling. + * - A character reference always covers a whole CODE POINT. Encoding one half + * of a surrogate pair destroys the character. + * + * Why these are VMark's handlers rather than upstream's: upstream's emphasis + * marker is one global option, and `delete` (mdast-util-gfm-strikethrough) does + * no flanking at all. Neighbour encoding itself still happens in upstream's + * `containerPhrasing`, driven by `attentionEncodeSurroundingInfo`. + * + * @coordinates-with serializer.ts — installs these handlers + * @coordinates-with markEdgeWhitespace.ts — moves edge whitespace out of `~~`, + * which cannot close after a space + * @module utils/markdownPipeline/serializerAttention + */ + +/** The slice of mdast-util-to-markdown's `State` these handlers use. */ +interface AttentionState { + enter: (construct: string) => () => void; + createTracker: (info: AttentionInfo) => { + move: (value: string) => string; + /** Position bookkeeping — `{ now, lineShift }`, NOT before/after. */ + current: () => { now: { line: number; column: number }; lineShift: number }; + }; + containerPhrasing: (node: unknown, info: { before: string; after: string }) => string; + /** Index of the child being serialized, per open container. */ + indexStack: number[]; + /** Read by `containerPhrasing` to encode the character on either side. */ + attentionEncodeSurroundingInfo?: { before: boolean; after: boolean }; +} + +/** The characters around the node being serialized. */ +interface AttentionInfo { + before: string; + after: string; +} + +interface PhrasingParent { + children?: ReadonlyArray<{ type: string }>; +} + +const WHITESPACE = 1; +const PUNCTUATION = 2; +type CharacterClass = typeof WHITESPACE | typeof PUNCTUATION | undefined; + +/** + * micromark's `classifyCharacter` for one UTF-16 code unit: whitespace, + * punctuation (Unicode P or S), or anything else. NaN — an empty neighbour — + * counts as "anything else", as upstream's does. + */ +function classify(code: number): CharacterClass { + if (Number.isNaN(code)) return undefined; + const char = String.fromCharCode(code); + if (/\s/.test(char)) return WHITESPACE; + if (/\p{P}|\p{S}/u.test(char)) return PUNCTUATION; + return undefined; +} + +/** + * Which side of one delimiter run must be character-referenced for it to form: + * mdast-util-to-markdown's `encodeInfo` table. `_` is stricter than `*` and `~` + * between two letters, where it cannot form at all. + */ +function encodeSides(outside: number, inside: number, marker: string): { inside: boolean; outside: boolean } { + const outsideClass = classify(outside); + const insideClass = classify(inside); + if (outsideClass === undefined) { + if (insideClass === undefined) { + return marker === "_" ? { inside: true, outside: true } : { inside: false, outside: false }; + } + return { inside: insideClass === WHITESPACE, outside: true }; + } + if (outsideClass === WHITESPACE) { + return insideClass === WHITESPACE ? { inside: true, outside: true } : { inside: false, outside: false }; + } + return { inside: insideClass === WHITESPACE, outside: false }; +} + +const reference = (codePoint: number): string => `&#x${codePoint.toString(16).toUpperCase()};`; + +/** `value` with its first code point written as a character reference. */ +function encodeFirstCodePoint(value: string): string { + const codePoint = value.codePointAt(0) ?? 0; + return reference(codePoint) + value.slice(codePoint > 0xffff ? 2 : 1); +} + +/** `value` with its last code point written as a character reference. */ +function encodeLastCodePoint(value: string): string { + const low = value.charCodeAt(value.length - 1); + const high = value.charCodeAt(value.length - 2); + const width = low >= 0xdc00 && low <= 0xdfff && high >= 0xd800 && high <= 0xdbff ? 2 : 1; + return value.slice(0, -width) + reference(value.codePointAt(value.length - width) ?? 0); +} + +/** Serialize one attention node wrapped in `delimiter`. */ +function serializeAttention( + node: unknown, + state: AttentionState, + info: AttentionInfo, + construct: string, + delimiter: string, +): string { + const marker = delimiter.charAt(0); + const exit = state.enter(construct); + const tracker = state.createTracker(info); + const before = tracker.move(delimiter); + let between = tracker.move( + state.containerPhrasing(node, { after: marker, before, ...tracker.current() }), + ); + + const open = encodeSides(info.before.charCodeAt(info.before.length - 1), between.charCodeAt(0), marker); + if (open.inside) between = encodeFirstCodePoint(between); + const close = encodeSides(info.after.charCodeAt(0), between.charCodeAt(between.length - 1), marker); + if (close.inside) between = encodeLastCodePoint(between); + + const after = tracker.move(delimiter); + exit(); + state.attentionEncodeSurroundingInfo = { before: open.outside, after: close.outside }; + return before + between + after; +} + +/** + * The position of `node` among its siblings. `containerPhrasing` records the + * current child in `indexStack` — the node itself when handling it, the node + * before it when peeking — so the common case is O(1). + */ +function siblingIndex(node: unknown, parent: PhrasingParent | undefined, state: AttentionState): number { + const siblings = parent?.children ?? []; + const current = state.indexStack[state.indexStack.length - 1] ?? -1; + if (siblings[current] === node) return current; + if (siblings[current + 1] === node) return current + 1; + return siblings.indexOf(node as { type: string }); +} + +/** `_` when a sibling `**` would otherwise sit flush against this emphasis. */ +function emphasisMarker(node: unknown, parent: PhrasingParent | undefined, state: AttentionState): "*" | "_" { + const index = siblingIndex(node, parent, state); + if (index < 0) return "*"; + const siblings = parent?.children ?? []; + return siblings[index - 1]?.type === "strong" || siblings[index + 1]?.type === "strong" ? "_" : "*"; +} + +/** `emphasis` handler; `peek` reports the marker `containerPhrasing` will see. */ +export const handleEmphasis = Object.assign( + (node: unknown, parent: PhrasingParent | undefined, state: AttentionState, info: AttentionInfo): string => + serializeAttention(node, state, info, "emphasis", emphasisMarker(node, parent, state)), + { + peek: (node: unknown, parent: PhrasingParent | undefined, state: AttentionState): string => + emphasisMarker(node, parent, state), + }, +); + +/** `strong` handler. */ +export const handleStrong = Object.assign( + (node: unknown, _parent: unknown, state: AttentionState, info: AttentionInfo): string => + serializeAttention(node, state, info, "strong", "**"), + { peek: (): string => "*" }, +); + +/** + * `delete` handler. GFM gives `~~` the same flanking test as `*` + * (micromark-extension-gfm-strikethrough), so it takes the same table. + */ +export const handleDelete = Object.assign( + (node: unknown, _parent: unknown, state: AttentionState, info: AttentionInfo): string => + serializeAttention(node, state, info, "strikethrough", "~~"), + { peek: (): string => "~" }, +); diff --git a/src/utils/markdownPipeline/serializerStrikethrough.ts b/src/utils/markdownPipeline/serializerStrikethrough.ts index 742e8456c..f3f8c0838 100644 --- a/src/utils/markdownPipeline/serializerStrikethrough.ts +++ b/src/utils/markdownPipeline/serializerStrikethrough.ts @@ -1,119 +1,16 @@ /** - * Strikethrough delimiter flanking — the `~~` half of what remark already does - * for `*`. + * Split-surrogate repair for attention neighbour encoding. * - * Purpose: stop `~~` delimiters being emitted where GFM cannot parse them back. + * Purpose: undo the damage `mdast-util-to-markdown` does when it character- + * references a delimiter's neighbour one UTF-16 code unit at a time. * - * GFM gives `~~` the same delimiter-run rules as emphasis: an OPENING run must - * be left-flanking, which it is not when it is followed by punctuation while - * being preceded by an alphanumeric; a CLOSING run must be right-flanking, the - * mirror image. So `plain~~* word~~tail` is not strikethrough at all — it - * reparses as literal text, and the tildes VMark emitted become four - * characters in the author's document that they never typed. That is text - * corruption, not a lost mark (audit 20260906, found by the editing fuzz once - * the mark-edge whitespace normalization stopped masking it). + * The `delete` handler that used to live here moved to serializerAttention.ts, + * which owns all three attention delimiters. * - * `mdast-util-to-markdown` solves this for emphasis and strong by CHARACTER- - * REFERENCING the offending neighbour — `plain*word*` becomes - * `plain*word*`, which is punctuation before the marker, so the run - * flanks and the text decodes back to exactly what it was. The strikethrough - * extension never adopted it, so `delete` was left with the raw handler. - * - * This is that same treatment, and deliberately nothing more: no change to - * which characters carry the mark (unlike the whitespace case, which genuinely - * cannot be represented and must move the boundary), and no HTML fallback - * (`` round-trips as `html_inline`, not as a strike mark — measured). - * - * @coordinates-with markEdgeWhitespace.ts — the whitespace half of the same rule - * @coordinates-with serializer.ts — installs this handler + * @coordinates-with serializer.ts — runs the repair on every serialization * @module utils/markdownPipeline/serializerStrikethrough */ -/** The `state` a `delete` handler receives from mdast-util-to-markdown. */ -interface DeleteState { - enter: (construct: string) => () => void; - createTracker: (info: unknown) => { - move: (value: string) => string; - /** Position bookkeeping — `{ now, lineShift }`, NOT before/after. */ - current: () => { now: { line: number; column: number }; lineShift: number }; - }; - containerPhrasing: ( - node: unknown, - info: { before: string; after: string }, - ) => string; - /** - * Consumed by `containerPhrasing` to character-reference the character on - * either side of this node. The mechanism emphasis uses; strikethrough - * simply never set it. - */ - attentionEncodeSurroundingInfo?: { before: boolean; after: boolean }; -} - -interface DeleteInfo { - before: string; - after: string; -} - -/** ASCII punctuation, per CommonMark's definition for delimiter flanking. */ -const PUNCTUATION = /[!-/:-@[-`{-~]/; -const WHITESPACE = /\s/; - -/** - * Whether the character OUTSIDE a delimiter run must be character-referenced - * for that run to flank. - * - * A run adjacent to punctuation only flanks when its outer neighbour is - * whitespace or punctuation. An empty `outside` means the start or end of the - * line, which counts as whitespace and always flanks. - */ -function outsideNeedsEncoding(outside: string, inside: string): boolean { - if (!inside || !PUNCTUATION.test(inside)) return false; - if (!outside) return false; - return !WHITESPACE.test(outside) && !PUNCTUATION.test(outside); -} - -/** - * Serialize a `delete` node as `~~…~~`, asking for the surrounding characters - * to be encoded when the delimiters would otherwise not flank. - */ -export function handleDelete( - node: unknown, - _parent: unknown, - state: DeleteState, - info: DeleteInfo, -): string { - const exit = state.enter("strikethrough"); - const tracker = state.createTracker(info); - tracker.move("~~"); - const between = tracker.move( - state.containerPhrasing(node, { - before: "~", - after: "~", - ...tracker.current(), - }), - ); - tracker.move("~~"); - exit(); - - const encodeBefore = outsideNeedsEncoding( - info.before.slice(-1), - between.slice(0, 1), - ); - const encodeAfter = outsideNeedsEncoding( - info.after.slice(0, 1), - between.slice(-1), - ); - if (encodeBefore || encodeAfter) { - state.attentionEncodeSurroundingInfo = { - before: encodeBefore, - after: encodeAfter, - }; - } - - return `~~${between}~~`; -} - - /** * Repair numeric character references that split an astral character. * From 16230682c9e0eac131c5b13061e54d79d26bde78 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 12:58:38 +0800 Subject: [PATCH 02/11] fix(markdown): encode attention neighbours as whole code points mdast-util-to-markdown makes a non-flanking delimiter work by writing its neighbour as a character reference, one UTF-16 code unit at a time. Beside an emoji that emitted `�` plus a raw low surrogate, or `��` with a delimiter on each side, and the character decoded to U+FFFD. VMark repaired the string afterwards. That saved the emoji but not its neighbour: the delimiter beside the pair had already decided it flanked a raw surrogate, which micromark reads as a letter, and after the repair it sat against `&`, which is punctuation. The soak's editing fuzz lost an italic that way (#1407, seed 7: `wordword*🙂**# ***`). The encoding now covers the whole code point at the moment it happens, in the patched containerPhrasing and in VMark's own attention handlers, so every flanking decision sees the final text. The string repair is deleted, and its module with it. A property test now pins the class rather than the shapes: bold, italic and strike runs over letters, CJK, RTL, emoji, ASCII and full-width punctuation and spaces must round-trip. A 40,000-case probe over that alphabet now fails only on a strike mark over a whitespace-only run, which markdown cannot spell and markEdgeWhitespace.ts deliberately moves out; the property excludes that shape by construction and says why. --- patches/mdast-util-to-markdown@2.1.2.patch | 47 ++++++++++ pnpm-lock.yaml | 20 ++-- .../attentionRoundtrip.property.test.ts | 89 ++++++++++++++++++ src/utils/markdownPipeline/serializer.ts | 11 +-- .../serializerAttention.astral.test.ts | 91 +++++++++++++++++++ ...serializerAttention.strikethrough.test.ts} | 76 +--------------- .../markdownPipeline/serializerAttention.ts | 3 +- .../serializerStrikethrough.ts | 74 --------------- 8 files changed, 246 insertions(+), 165 deletions(-) create mode 100644 src/utils/markdownPipeline/__tests__/attentionRoundtrip.property.test.ts create mode 100644 src/utils/markdownPipeline/serializerAttention.astral.test.ts rename src/utils/markdownPipeline/{serializerStrikethrough.test.ts => serializerAttention.strikethrough.test.ts} (60%) delete mode 100644 src/utils/markdownPipeline/serializerStrikethrough.ts diff --git a/patches/mdast-util-to-markdown@2.1.2.patch b/patches/mdast-util-to-markdown@2.1.2.patch index aff5925de..04a8ccfbb 100644 --- a/patches/mdast-util-to-markdown@2.1.2.patch +++ b/patches/mdast-util-to-markdown@2.1.2.patch @@ -1,3 +1,50 @@ +diff --git a/lib/util/container-phrasing.js b/lib/util/container-phrasing.js +index 3b4dc46bcab677d1a86c0ca3cc0d11e876e7e81b..7b42522d79ac84ee7ef58c12345fac67a8cd6c16 100644 +--- a/lib/util/container-phrasing.js ++++ b/lib/util/container-phrasing.js +@@ -88,9 +88,18 @@ export function containerPhrasing(parent, state, info) { + // If we had to encode the first character after the previous node and it’s + // still the same character, + // encode it. ++ // ++ // VMark patch (#1407 soak): encode the whole CODE POINT, not one UTF-16 ++ // code unit. Encoding only the high surrogate of an astral character (an ++ // emoji, most CJK Extension B) emits `�` plus a raw low surrogate, ++ // which decodes to U+FFFD: the character is destroyed. Repairing that ++ // afterwards in the string is too late, because the attention handler ++ // beside it has already decided its flanking against the raw surrogate. + if (encodeAfter && encodeAfter === value.slice(0, 1)) { ++ const codePoint = /** @type {number} */ (value.codePointAt(0)) + value = +- encodeCharacterReference(encodeAfter.charCodeAt(0)) + value.slice(1) ++ encodeCharacterReference(codePoint) + ++ value.slice(codePoint > 0xffff ? 2 : 1) + } + + const encodingInfo = state.attentionEncodeSurroundingInfo +@@ -106,9 +115,20 @@ export function containerPhrasing(parent, state, info) { + encodingInfo.before && + before === results[results.length - 1].slice(-1) + ) { ++ // VMark patch (#1407 soak): the whole code point, as above — here the ++ // raw half left behind would be the high surrogate. ++ const previous = results[results.length - 1] ++ const low = previous.charCodeAt(previous.length - 1) ++ const high = previous.charCodeAt(previous.length - 2) ++ const width = ++ low >= 0xdc00 && low <= 0xdfff && high >= 0xd800 && high <= 0xdbff ++ ? 2 ++ : 1 + results[results.length - 1] = +- results[results.length - 1].slice(0, -1) + +- encodeCharacterReference(before.charCodeAt(0)) ++ previous.slice(0, -width) + ++ encodeCharacterReference( ++ /** @type {number} */ (previous.codePointAt(previous.length - width)) ++ ) + } + + if (encodingInfo.after) encodeAfter = after diff --git a/lib/util/safe.js b/lib/util/safe.js index 456fe215ae3d032ffaea4590f0d097bcb9195619..10f82c99cff8123142f230c087e6d09742f2b313 100644 --- a/lib/util/safe.js diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1423b900c..0b056387f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -32,7 +32,7 @@ overrides: patchedDependencies: mdast-util-to-markdown@2.1.2: - hash: 2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf + hash: fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773 path: patches/mdast-util-to-markdown@2.1.2.patch micromark@4.0.2: hash: c4162f5915a6a3308e12ec56d2a8ef6a3a9cbaa7abea611d0ce5957fc4f620ea @@ -16711,7 +16711,7 @@ snapshots: devlop: 1.1.0 escape-string-regexp: 5.0.0 mdast-util-from-markdown: 2.0.3 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) micromark-extension-frontmatter: 2.0.0 transitivePeerDependencies: - supports-color @@ -16729,7 +16729,7 @@ snapshots: '@types/mdast': 4.0.4 devlop: 1.1.0 mdast-util-from-markdown: 2.0.3 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) micromark-util-normalize-identifier: 2.0.1 transitivePeerDependencies: - supports-color @@ -16738,7 +16738,7 @@ snapshots: dependencies: '@types/mdast': 4.0.4 mdast-util-from-markdown: 2.0.3 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) transitivePeerDependencies: - supports-color @@ -16748,7 +16748,7 @@ snapshots: devlop: 1.1.0 markdown-table: 3.0.4 mdast-util-from-markdown: 2.0.3 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) transitivePeerDependencies: - supports-color @@ -16757,7 +16757,7 @@ snapshots: '@types/mdast': 4.0.4 devlop: 1.1.0 mdast-util-from-markdown: 2.0.3 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) transitivePeerDependencies: - supports-color @@ -16769,7 +16769,7 @@ snapshots: mdast-util-gfm-strikethrough: 2.0.0 mdast-util-gfm-table: 2.0.0 mdast-util-gfm-task-list-item: 2.0.0 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) transitivePeerDependencies: - supports-color @@ -16780,7 +16780,7 @@ snapshots: devlop: 1.1.0 longest-streak: 3.1.0 mdast-util-from-markdown: 2.0.3 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) unist-util-remove-position: 5.0.0 transitivePeerDependencies: - supports-color @@ -16807,7 +16807,7 @@ snapshots: unist-util-visit: 5.1.0 vfile: 6.0.3 - mdast-util-to-markdown@2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf): + mdast-util-to-markdown@2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773): dependencies: '@types/mdast': 4.0.4 '@types/unist': 3.0.3 @@ -17984,7 +17984,7 @@ snapshots: remark-stringify@11.0.0: dependencies: '@types/mdast': 4.0.4 - mdast-util-to-markdown: 2.1.2(patch_hash=2dbee866e6cf244b099fa367885b1e8ad51ff315b895c1ad77581dee0f4206bf) + mdast-util-to-markdown: 2.1.2(patch_hash=fac6c36ab79a787b23857dabd34de41ab06dc7b3e76943769c0d4177b0bae773) unified: 11.0.5 require-directory@2.1.1: {} diff --git a/src/utils/markdownPipeline/__tests__/attentionRoundtrip.property.test.ts b/src/utils/markdownPipeline/__tests__/attentionRoundtrip.property.test.ts new file mode 100644 index 000000000..66108507e --- /dev/null +++ b/src/utils/markdownPipeline/__tests__/attentionRoundtrip.property.test.ts @@ -0,0 +1,89 @@ +// @vitest-environment node +/** + * Property: bold, italic and strikethrough survive a save, whatever sits beside + * their delimiters. + * + * The soak's editing fuzz found three separate ways a delimiter was emitted + * where micromark cannot read it back — merged `*` runs, a split surrogate + * pair, and punctuation classified as ASCII-only — each one after the others + * had been fixed by example. Examples pin a shape; this pins the class: runs of + * marked text drawn from the characters that decide flanking (letters, CJK, + * RTL, emoji, ASCII and full-width punctuation, spaces), in every mark + * combination. + * + * Whitespace-only runs are excluded, and that is a statement about markdown, + * not a convenience: `~~ ~~` has no spelling, so VMark moves whitespace out of + * a strikethrough (markEdgeWhitespace.ts). Every run here carries at least one + * non-space character; edge spaces are still generated. + */ +import { describe, expect, it } from "vitest"; +import fc from "fast-check"; +import type { Node as PMNode } from "@tiptap/pm/model"; +import { getProductionSchema } from "@/test/productionSchema"; +import { parseMarkdown, serializeMarkdown } from "../adapter"; + +const schema = getProductionSchema(); +const MARKS = ["bold", "italic", "strike"] as const; +const CHARS = ["a", "d", "é", "1", "中", "文", "ע", "🙂", "#", ".", "*", "。", "(", " "]; + +/** Same budget reasoning as roundtrip.property.test.ts: a liveness bound only. */ +const PROPERTY_TEST_TIMEOUT_MS = 30_000; + +const run = fc.record({ + text: fc + .array(fc.constantFrom(...CHARS), { minLength: 1, maxLength: 3 }) + .map((chars) => chars.join("")) + .filter((text) => text.trim() !== ""), + marks: fc.subarray([...MARKS]), +}); + +type Run = { text: string; marks: readonly string[] }; + +/** + * Text as runs of [text, marks], with edge spaces moved out of marked runs — + * the one normalization markdown forces (a closer cannot follow a space). + */ +function canonicalRuns(doc: PMNode): Array<[string, string]> { + const out: Array<[string, string]> = []; + const push = (text: string, marks: string) => { + if (!text) return; + const prev = out[out.length - 1]; + if (prev && prev[1] === marks) prev[0] += text; + else out.push([text, marks]); + }; + doc.descendants((node) => { + if (!node.isText || !node.text) return; + const marks = node.marks.map((m) => m.type.name).sort().join("+"); + if (!marks) return push(node.text, ""); + const [, lead, core, trail] = /^( *)(.*?)( *)$/su.exec(node.text) ?? ["", "", node.text, ""]; + push(lead, ""); + push(core, marks); + push(trail, ""); + }); + // Line-edge spaces are not representable either. + if (out.length) { + out[0][0] = out[0][0].replace(/^ +/, ""); + out[out.length - 1][0] = out[out.length - 1][0].replace(/ +$/, ""); + } + return out.filter(([text]) => text !== ""); +} + +describe("attention delimiters — round-trip property", () => { + it("preserves every bold/italic/strike run through serialize and parse", () => { + fc.assert( + fc.property(fc.array(run, { minLength: 1, maxLength: 5 }), (runs: Run[]) => { + const doc = schema.node("doc", null, [ + schema.node( + "paragraph", + null, + runs.map((r) => schema.text(r.text, r.marks.map((m) => schema.marks[m].create()))), + ), + ]); + const markdown = serializeMarkdown(schema, doc); + const back = parseMarkdown(schema, markdown); + expect(canonicalRuns(back), `markdown: ${JSON.stringify(markdown)}`).toEqual(canonicalRuns(doc)); + }), + { numRuns: 400, seed: 20260915 }, + ); + }, PROPERTY_TEST_TIMEOUT_MS); +}); diff --git a/src/utils/markdownPipeline/serializer.ts b/src/utils/markdownPipeline/serializer.ts index f1eb503b2..378cf889f 100644 --- a/src/utils/markdownPipeline/serializer.ts +++ b/src/utils/markdownPipeline/serializer.ts @@ -29,7 +29,6 @@ import { unified } from "unified"; import remarkStringify from "remark-stringify"; -import { repairSplitSurrogateEntities } from "./serializerStrikethrough"; import { handleDelete, handleEmphasis, handleStrong } from "./serializerAttention"; import remarkGfm from "remark-gfm"; import remarkMath from "remark-math"; @@ -117,12 +116,10 @@ export function serializeMdastToMarkdown( ): string { const processor = getSerializer(); let result = processor.stringify(mdast); - - // Correctness, not cosmetics: the attention-encoding that makes a delimiter - // flank splits an astral neighbour across its surrogate pair, destroying the - // character. Repaired here — before every other pass, and with no size - // ceiling. - result = repairSplitSurrogateEntities(result); + // No split-surrogate repair pass: attention neighbours are encoded as whole + // code points when they are encoded (serializerAttention.ts and the + // mdast-util-to-markdown patch), which a string repair afterwards could not + // do without changing what the delimiter beside them flanks. // A document-leading thematic break can serialize as `---` and then be // REPARSED as a frontmatter fence, swallowing structure (CommonMark diff --git a/src/utils/markdownPipeline/serializerAttention.astral.test.ts b/src/utils/markdownPipeline/serializerAttention.astral.test.ts new file mode 100644 index 000000000..4db22669c --- /dev/null +++ b/src/utils/markdownPipeline/serializerAttention.astral.test.ts @@ -0,0 +1,91 @@ +// @vitest-environment node +/** + * An astral character beside an attention delimiter survives a save. + * + * A delimiter that would not flank is fixed by writing its neighbour as a + * character reference. mdast-util-to-markdown did that one UTF-16 CODE UNIT at + * a time, so an emoji came out as `�` plus a raw low surrogate — or, with + * a delimiter on each side, as `��` — and decoded to U+FFFD. + * + * VMark used to patch the string afterwards (`repairSplitSurrogateEntities`), + * which saved the emoji but was too late for its neighbours: the delimiter + * beside the pair had already decided it flanked a raw surrogate, a letter to + * the parser, and after the repair it sat against `&`, which is punctuation. + * The soak's editing fuzz lost an italic that way (#1407, seed 7). The encoding + * now covers the whole code point at the moment it happens (the + * mdast-util-to-markdown patch), so every flanking decision sees the final text. + */ +import { describe, expect, it } from "vitest"; +import type { Node as PMNode } from "@tiptap/pm/model"; +import { getProductionSchema } from "@/test/productionSchema"; +import { parseMarkdown, serializeMarkdown } from "./adapter"; + +const schema = getProductionSchema(); + +type MarkName = "bold" | "italic" | "strike"; +type Run = [text: string, marks: MarkName[]]; + +function roundTrip(runs: Run[]): { markdown: string; runs: Run[]; text: string } { + const doc = schema.node("doc", null, [ + schema.node( + "paragraph", + null, + runs.map(([text, marks]) => schema.text(text, marks.map((m) => schema.marks[m].create()))), + ), + ]); + const markdown = serializeMarkdown(schema, doc); + const reparsed: PMNode = parseMarkdown(schema, markdown); + const out: Run[] = []; + reparsed.descendants((node) => { + if (!node.isText) return; + const marks = node.marks.map((m) => m.type.name as MarkName).sort(); + const prev = out[out.length - 1]; + if (prev && prev[1].join() === marks.join()) prev[0] += node.text ?? ""; + else out.push([node.text ?? "", marks]); + }); + return { markdown, runs: out, text: reparsed.textContent }; +} + +describe("astral characters beside an encoded delimiter", () => { + // Seed 7: `wordword*🙂**# ***` — the italic opener had been + // checked against the raw high surrogate, then the repair put `&` after it. + it("keeps an italic that opens on an emoji followed by bold (seed 7)", () => { + const runs: Run[] = [["wordword", []], ["🙂", ["italic"]], ["#", ["bold", "italic"]]]; + expect(roundTrip(runs).runs).toEqual(runs); + }); + + // A delimiter on EACH side of one emoji encoded each half separately. + it.each<[string, Run[]]>([ + ["bold and strike", [[".", ["bold"]], ["🙂", []], [".", ["strike"]]]], + ["bold and bold+italic", [["🙂", ["bold", "strike"]], ["🙂", []], ["d", ["bold", "italic", "strike"]]]], + ])("keeps an emoji between two delimiters: %s", (_label, runs) => { + const back = roundTrip(runs); + expect(back.text).not.toContain("�"); + expect(back.runs).toEqual(runs.map(([t, m]) => [t, [...m].sort()])); + }); + + it.each<[MarkName, string]>([ + ["bold", "word*"], + ["italic", "word*"], + ["strike", "word*"], + ])("preserves an emoji after a %s run ending in punctuation", (mark, marked) => { + const back = roundTrip([[marked, [mark]], ["🙂word", []]]); + expect(back.text).toBe("word*🙂word"); + expect(back.runs).toEqual([[marked, [mark]], ["🙂word", []]]); + }); + + it("writes the pair as one reference for the code point", () => { + const { markdown } = roundTrip([["word*", ["bold"]], ["🙂word", []]]); + expect(markdown).toContain("🙂"); + expect(markdown).not.toMatch(/ [89A-F][0-9A-F]{2};/i); + }); + + // `_` letter/letter encodes the INSIDE character too — the handler's own + // encoding, which must be code-point aware as well. + it("keeps an emoji inside italic spelled `_` between letters", () => { + const runs: Run[] = [["a", ["bold"]], ["🙂", ["italic"]], ["b", []]]; + const back = roundTrip(runs); + expect(back.runs).toEqual(runs); + expect(back.markdown).not.toMatch(/ [89A-F][0-9A-F]{2};/i); + }); +}); diff --git a/src/utils/markdownPipeline/serializerStrikethrough.test.ts b/src/utils/markdownPipeline/serializerAttention.strikethrough.test.ts similarity index 60% rename from src/utils/markdownPipeline/serializerStrikethrough.test.ts rename to src/utils/markdownPipeline/serializerAttention.strikethrough.test.ts index 585a01b18..b0ac82df7 100644 --- a/src/utils/markdownPipeline/serializerStrikethrough.test.ts +++ b/src/utils/markdownPipeline/serializerAttention.strikethrough.test.ts @@ -7,11 +7,13 @@ * stopped masking it. The failing shape is the one GFM's flanking rules * forbid: an opening run followed by punctuation while preceded by an * alphanumeric (and its mirror image at the closer). + * + * The handler lives in serializerAttention.ts; astral neighbours are covered by + * serializerAttention.astral.test.ts. */ import { describe, expect, it } from "vitest"; import { getProductionSchema } from "@/test/productionSchema"; import { serializeMarkdown, parseMarkdown } from "./adapter"; -import { repairSplitSurrogateEntities } from "./serializerStrikethrough"; const schema = getProductionSchema(); @@ -107,75 +109,3 @@ describe("strikethrough delimiter flanking", () => { }); }); - -/** - * Audit 20260906 — attention-encoding split astral characters. - * - * `mdast-util-to-markdown` makes a non-flanking delimiter work by character- - * referencing its neighbour, but encodes by UTF-16 CODE UNIT: an emoji next to - * such a delimiter came out as `�` plus a raw low surrogate, reparsing - * as U+FFFD. Upstream, and older than VMark's `delete` handler — plain - * `**bold**` reproduces it with no strikethrough involved. - */ -describe("astral characters beside an encoded delimiter", () => { - /** Round-trip a paragraph of [marked, plain]. */ - function afterMark(markName: "bold" | "italic" | "strike", marked: string, tail: string) { - const doc = schema.node("doc", null, [ - schema.node("paragraph", null, [ - schema.text(marked, [schema.marks[markName].create()]), - schema.text(tail), - ]), - ]); - const markdown = serializeMarkdown(schema, doc); - return { markdown, text: parseMarkdown(schema, markdown).textContent }; - } - - it("preserves an emoji after a bold run ending in punctuation", () => { - const { text } = afterMark("bold", "word*", "🙂word"); - - expect(text).toBe("word*🙂word"); - expect(text).not.toContain("\uFFFD"); - }); - - it("preserves an emoji after a strikethrough run ending in punctuation", () => { - const { text } = afterMark("strike", "word*", "🙂word"); - - expect(text).toBe("word*🙂word"); - }); - - it("preserves an emoji after an italic run ending in punctuation", () => { - const { text } = afterMark("italic", "word*", "🙂word"); - - expect(text).toBe("word*🙂word"); - }); - - // The repair must re-encode the PAIR, not decode it — decoding would undo - // the very thing that makes the delimiter flank. - it("re-encodes the surrogate pair as one code point", () => { - expect(afterMark("bold", "word*", "🙂word").markdown).toContain("🙂"); - }); - - describe("repairSplitSurrogateEntities", () => { - it("joins a high-surrogate reference with its raw low surrogate", () => { - expect(repairSplitSurrogateEntities("a�\uDE42b")).toBe("a🙂b"); - }); - - it("leaves an ordinary character reference alone", () => { - expect(repairSplitSurrogateEntities("plain~~x~~")).toBe("plain~~x~~"); - }); - - it("leaves text with no references alone", () => { - expect(repairSplitSurrogateEntities("just 🙂 text")).toBe("just 🙂 text"); - }); - - it("ignores a high-surrogate reference not followed by a low surrogate", () => { - expect(repairSplitSurrogateEntities("�x")).toBe("�x"); - }); - - it("repairs several occurrences", () => { - expect(repairSplitSurrogateEntities("�\uDE42 and �\uDE00")).toBe( - "🙂 and 😀", - ); - }); - }); -}); diff --git a/src/utils/markdownPipeline/serializerAttention.ts b/src/utils/markdownPipeline/serializerAttention.ts index 9d6c7242a..ad78ccd50 100644 --- a/src/utils/markdownPipeline/serializerAttention.ts +++ b/src/utils/markdownPipeline/serializerAttention.ts @@ -28,7 +28,8 @@ * Why these are VMark's handlers rather than upstream's: upstream's emphasis * marker is one global option, and `delete` (mdast-util-gfm-strikethrough) does * no flanking at all. Neighbour encoding itself still happens in upstream's - * `containerPhrasing`, driven by `attentionEncodeSurroundingInfo`. + * `containerPhrasing`, driven by `attentionEncodeSurroundingInfo`, which VMark + * patches to encode whole code points too (patches/mdast-util-to-markdown). * * @coordinates-with serializer.ts — installs these handlers * @coordinates-with markEdgeWhitespace.ts — moves edge whitespace out of `~~`, diff --git a/src/utils/markdownPipeline/serializerStrikethrough.ts b/src/utils/markdownPipeline/serializerStrikethrough.ts deleted file mode 100644 index f3f8c0838..000000000 --- a/src/utils/markdownPipeline/serializerStrikethrough.ts +++ /dev/null @@ -1,74 +0,0 @@ -/** - * Split-surrogate repair for attention neighbour encoding. - * - * Purpose: undo the damage `mdast-util-to-markdown` does when it character- - * references a delimiter's neighbour one UTF-16 code unit at a time. - * - * The `delete` handler that used to live here moved to serializerAttention.ts, - * which owns all three attention delimiters. - * - * @coordinates-with serializer.ts — runs the repair on every serialization - * @module utils/markdownPipeline/serializerStrikethrough - */ - -/** - * Repair numeric character references that split an astral character. - * - * `mdast-util-to-markdown` fixes a non-flanking delimiter by character- - * referencing the neighbour, but it does so by UTF-16 CODE UNIT. When that - * neighbour is an astral character — an emoji, most CJK extension-B - * ideographs — it encodes only the leading HIGH SURROGATE and leaves the low - * surrogate raw: - * - * **word\***🙂word → **word\***�\uDE42word - * - * which reparses as U+FFFD followed by a lone low surrogate. The emoji is - * destroyed. It is upstream, it predates VMark's `delete` handler — plain - * `**bold**` ending in punctuation and followed by an emoji reproduces it with - * no strikethrough anywhere — and it is real text corruption, not a cosmetic - * artefact (audit 20260906, found by the editing fuzz). - * - * The repair is to finish the job the library started: re-encode the PAIR as - * one reference for the actual code point. Decoding back to the raw character - * would be wrong — the encoding is what makes the delimiter flank, so undoing - * it would break the emphasis instead. - * - * BOTH directions occur. Which half gets encoded depends on which side of the - * delimiter the astral character sits: encoding the neighbour AFTER a closer - * takes its first code unit (the high surrogate), while encoding the neighbour - * BEFORE an opener takes its last (the low surrogate). - * - * Applied unconditionally, and NOT in the cosmetic pass: that pass is skipped - * above a size ceiling, and a correctness repair must not have one. - */ -export function repairSplitSurrogateEntities(markdown: string): string { - if (!markdown.includes("&#x")) return markdown; - - /** One reference for the code point the pair encodes. */ - const joined = (high: number, low: number): string => - `&#x${(((high - 0xd800) * 0x400 + (low - 0xdc00) + 0x10000) - .toString(16) - .toUpperCase())};`; - - return ( - markdown - // Encoded HIGH surrogate followed by a raw low one. - .replace( - /&#x(D[89ab][0-9a-f]{2});([\uDC00-\uDFFF])/gi, - (whole, hex: string, low: string) => { - const high = Number.parseInt(hex, 16); - if (high < 0xd800 || high > 0xdbff) return whole; - return joined(high, low.charCodeAt(0)); - }, - ) - // Raw HIGH surrogate followed by an encoded low one. - .replace( - /([\uD800-\uDBFF])&#x(D[c-f][0-9a-f]{2});/gi, - (whole, high: string, hex: string) => { - const low = Number.parseInt(hex, 16); - if (low < 0xdc00 || low > 0xdfff) return whole; - return joined(high.charCodeAt(0), low); - }, - ) - ); -} From 7b4c3cfbd5f03c63c27747d7332b18c673981ec9 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 13:06:28 +0800 Subject: [PATCH 03/11] fix(markdown): put a blank line before a list that cannot interrupt a paragraph CommonMark 5.2 lets a list interrupt a paragraph only if its first item does not start with a blank line and, when ordered, starts at 1. Upstream joins the children of a tight list item with no blank line, so an empty nested item written under a paragraph was read back as that paragraph's text: the soak's editing fuzz turned an empty nested ordered item into the characters "1." (#1407, seed 3: `1. **b**\n 1.`). A nested ordered list starting at 0 or 3 was lost the same way. A new join adds the blank line exactly when the pair would otherwise break, and keeps a longer captured blank-line run (ADR-1a). A list that can interrupt keeps its tight spelling, and an empty paragraph, which writes no text, never triggers it. Bullets follow the same rule, and that changes one pinned expectation. `- Parent text\n -` round-tripped in VMark only because the parser inserts the blank line itself on input (normalizeBareListMarkers); a CommonMark reader such as markdown-it renders it as `

Parent text

`. The serializer now writes `- Parent text\n\n -`, which both read as the nested empty item, and adapter.test.ts asserts that spelling and its stability. blankLinesJoin's parameter type gains `| undefined` so an mdast node satisfies it under exactOptionalPropertyTypes. The serializer header also names the attention and join modules it now coordinates with. --- .../__tests__/adapter.test.ts | 11 +- .../listInterruptJoin.test.ts | 118 ++++++++++++++++++ .../markdownPipeline/listInterruptJoin.ts | 53 ++++++++ src/utils/markdownPipeline/serializer.ts | 11 +- .../markdownPipeline/serializerHandlers.ts | 2 +- 5 files changed, 189 insertions(+), 6 deletions(-) create mode 100644 src/utils/markdownPipeline/listInterruptJoin.test.ts create mode 100644 src/utils/markdownPipeline/listInterruptJoin.ts diff --git a/src/utils/markdownPipeline/__tests__/adapter.test.ts b/src/utils/markdownPipeline/__tests__/adapter.test.ts index 45aef15ea..c5c8528aa 100644 --- a/src/utils/markdownPipeline/__tests__/adapter.test.ts +++ b/src/utils/markdownPipeline/__tests__/adapter.test.ts @@ -244,9 +244,14 @@ describe("serializeMarkdown", () => { expect(result).toContain("Parent text"); // Should still be a list expect(result).toMatch(/^- /m); - // Should NOT have a blank line between parent text and nested item - // (normalizeBareListMarkers inserts one for parsing, but spread fix removes it) - expect(result).not.toMatch(/Parent text\n\n/); + // The blank line CommonMark §5.2 requires before an empty item that + // follows a paragraph. VMark's parser inserts it itself on input + // (normalizeBareListMarkers), which is why the tight spelling used to + // round-trip here, but a CommonMark reader does not: markdown-it renders + // `- Parent text\n -` as `

Parent text

`. The serializer now + // writes it (listInterruptJoin.ts), and the result is stable. + expect(result).toBe("- Parent text\n\n -\n"); + expect(serializeMarkdown(schema, parseMarkdown(schema, result))).toBe(result); }); }); }); diff --git a/src/utils/markdownPipeline/listInterruptJoin.test.ts b/src/utils/markdownPipeline/listInterruptJoin.test.ts new file mode 100644 index 000000000..e64d4ee1f --- /dev/null +++ b/src/utils/markdownPipeline/listInterruptJoin.test.ts @@ -0,0 +1,118 @@ +// @vitest-environment node +/** + * A list that cannot interrupt a paragraph is separated from it by a blank line. + * + * CommonMark §5.2: when the first item of a list would start on a line that + * could continue a paragraph, it may interrupt that paragraph only if it does + * not start with a blank line and, if ordered, starts at 1. Inside a tight list + * item a nested list is joined to the paragraph above it with NO blank line, so + * an empty first item — `1. **b**\n 1.` — or a `3.` start was read back as + * paragraph text. The soak's editing fuzz found it as an empty nested ordered + * item turning into the characters "1." (#1407, seed 3). + * + * Bullets are in the same rule. micromark happens to accept `- b\n -`, but a + * CommonMark parser reads that `-` as a setext underline and the item becomes a + * heading, so the file was only readable by VMark. + */ +import { describe, expect, it } from "vitest"; +import type { List, ListItem, Paragraph, Root } from "mdast"; +import { getProductionSchema } from "@/test/productionSchema"; +import { parseMarkdown, serializeMarkdown } from "./adapter"; +import { serializeMdastToMarkdown } from "./serializer"; + +const schema = getProductionSchema(); +const { nodes, marks } = schema; + +const p = (text?: string) => + nodes.paragraph.create(null, text ? [schema.text(text)] : []); +const li = (...content: ReturnType[]) => nodes.listItem.create(null, content); + +/** Round-trip a document and return [markdown, reparsed JSON, original JSON]. */ +function roundTrip(doc: ReturnType) { + const markdown = serializeMarkdown(schema, doc); + const strip = (json: unknown): unknown => + JSON.parse( + JSON.stringify(json, (key, value: unknown) => + key === "sourceLine" || key === "blankLinesBefore" ? undefined : value, + ), + ); + return { + markdown, + back: strip(parseMarkdown(schema, markdown).toJSON()), + want: strip(doc.toJSON()), + }; +} + +describe("nested list after a paragraph in a list item", () => { + // Seed 3, reduced: an empty nested ordered item under "**b**". + it("keeps an empty nested ordered item (seed 3)", () => { + const nested = nodes.orderedList.create({ start: 1 }, [li(p())]); + const bold = nodes.paragraph.create(null, [schema.text("b", [marks.bold.create()])]); + const doc = nodes.doc.create(null, [ + nodes.orderedList.create({ start: 1 }, [nodes.listItem.create(null, [bold, nested])]), + ]); + const { markdown, back, want } = roundTrip(doc); + expect(back).toEqual(want); + expect(markdown).toBe("1. **b**\n\n 1.\n"); + }); + + it("separates an empty nested bullet item so CommonMark does not read a heading", () => { + const doc = nodes.doc.create(null, [ + nodes.bulletList.create(null, [li(p("b"), nodes.bulletList.create(null, [li(p())]))]), + ]); + const { markdown, back, want } = roundTrip(doc); + expect(back).toEqual(want); + expect(markdown).toBe("- b\n\n -\n"); + }); + + it.each([0, 3])("keeps a nested ordered list that starts at %i", (start) => { + const doc = nodes.doc.create(null, [ + nodes.bulletList.create(null, [li(p("b"), nodes.orderedList.create({ start }, [li(p("c"))]))]), + ]); + const { markdown, back, want } = roundTrip(doc); + expect(back).toEqual(want); + expect(markdown).toBe(`- b\n\n ${start}. c\n`); + }); + + // The rule is narrow on purpose: a list that CAN interrupt keeps the tight + // spelling authors wrote, so no existing document gains blank lines. + it("leaves a list that can interrupt the paragraph tight", () => { + const doc = nodes.doc.create(null, [ + nodes.bulletList.create(null, [ + li(p("b"), nodes.orderedList.create({ start: 1 }, [li(p("c")), li(p())])), + ]), + ]); + expect(roundTrip(doc).markdown).toBe("- b\n 1. c\n 2.\n"); + }); + + // An empty paragraph writes no text, so there is nothing to interrupt. + it("does not add a blank line after an empty paragraph", () => { + const doc = nodes.doc.create(null, [ + nodes.bulletList.create(null, [li(p(), nodes.bulletList.create(null, [li(p()), li(p("x"))]))]), + ]); + expect(roundTrip(doc).markdown).toBe("-\n -\n - x\n"); + }); +}); + +describe("captured blank lines before a list that cannot interrupt", () => { + const paragraph: Paragraph = { type: "paragraph", children: [{ type: "text", value: "b" }] }; + const emptyItemList = (blankLinesBefore?: number): List => ({ + type: "list", + ordered: false, + spread: false, + children: [{ type: "listItem", spread: false, children: [{ type: "paragraph", children: [] }] } satisfies ListItem], + ...(blankLinesBefore === undefined ? {} : { data: { blankLinesBefore } }), + }); + const serialize = (list: List) => + serializeMdastToMarkdown({ type: "root", children: [paragraph, list] } satisfies Root); + + // preserveBlankLines can capture 0 from a source where the list DID + // interrupt, and an edit can then empty its first item. + it("raises a captured 0 to the one blank line the list needs", () => { + expect(serialize(emptyItemList(0))).toBe("b\n\n-\n"); + }); + + it("keeps a captured run longer than one", () => { + expect(serialize(emptyItemList(2))).toBe("b\n\n\n-\n"); + }); +}); diff --git a/src/utils/markdownPipeline/listInterruptJoin.ts b/src/utils/markdownPipeline/listInterruptJoin.ts new file mode 100644 index 000000000..cda10d8c1 --- /dev/null +++ b/src/utils/markdownPipeline/listInterruptJoin.ts @@ -0,0 +1,53 @@ +/** + * Blank line before a list that cannot interrupt a paragraph. + * + * Purpose: an mdast-util-to-markdown `join` that keeps a list from being read + * back as the text of the paragraph above it. + * + * CommonMark §5.2: a list whose first item would start on a paragraph + * continuation line may interrupt that paragraph only if the item does not + * start with a blank line and, when ordered, starts at 1. Upstream joins the + * children of a tight list item with no blank line, so a nested empty item + * (`1. **b**\n 1.`) or a `3.` start became paragraph text on reparse — the + * soak's editing fuzz lost an empty nested ordered item that way (#1407, + * seed 3). Bullets obey the same rule: micromark tolerates `- b\n -`, but a + * CommonMark parser reads the `-` as a setext underline. + * + * Key decisions: + * - Narrow by construction: the join says nothing unless the pair would + * break, so every list that can interrupt keeps its tight spelling. + * - An EMPTY paragraph writes no text and has nothing to interrupt. + * - A captured blank-line run (blankLinesJoin, ADR-1a) is honoured when it is + * at least one line; a captured 0 is raised to the 1 the list needs. + * + * @coordinates-with serializer.ts — registers this join after blankLinesJoin, + * so it is consulted first + * @coordinates-with serializerHandlers.ts — blankLinesJoin, whose count it keeps + * @module utils/markdownPipeline/listInterruptJoin + */ +import type { List, Nodes } from "mdast"; +import { blankLinesJoin } from "./serializerHandlers"; + +/** Whether a list item's first line is blank: no content, or an empty paragraph. */ +function startsWithBlankLine(item: Nodes | undefined): boolean { + if (!item || item.type !== "listItem") return false; + const first = item.children[0]; + return first === undefined || (first.type === "paragraph" && first.children.length === 0); +} + +/** Whether CommonMark forbids `list` from interrupting a paragraph. */ +function cannotInterruptParagraph(list: List): boolean { + if (list.ordered && (list.start ?? 1) !== 1) return true; + return startsWithBlankLine(list.children[0]); +} + +/** + * Join: at least one blank line between a non-empty paragraph and a list that + * could not interrupt it; `undefined` (defer to the other joins) otherwise. + */ +export function listInterruptJoin(left: Nodes, right: Nodes): number | undefined { + if (left.type !== "paragraph" || left.children.length === 0) return undefined; + if (right.type !== "list" || !cannotInterruptParagraph(right)) return undefined; + const captured = blankLinesJoin(left, right); + return captured !== undefined && captured >= 1 ? captured : 1; +} diff --git a/src/utils/markdownPipeline/serializer.ts b/src/utils/markdownPipeline/serializer.ts index 378cf889f..1358888dc 100644 --- a/src/utils/markdownPipeline/serializer.ts +++ b/src/utils/markdownPipeline/serializer.ts @@ -19,11 +19,15 @@ * exact same mdast as the conservative output, so it can never change * document meaning (audit H6/H7). * - hardBreakStyle option converts `\` breaks to two-space breaks - * - join re-emits captured blank-line runs (blankLinesJoin, ADR-1a) + * - join re-emits captured blank-line runs (blankLinesJoin, ADR-1a), and + * keeps a list that cannot interrupt a paragraph off its last line + * (listInterruptJoin, CommonMark §5.2) * * @coordinates-with parser.ts — plugins must match between parser and serializer * @coordinates-with adapter.ts — wraps this with error handling * @coordinates-with serializerHandlers.ts — custom image/link to-markdown handlers + * @coordinates-with serializerAttention.ts — emphasis/strong/delete handlers + * @coordinates-with listInterruptJoin.ts — blank line before a non-interrupting list * @module utils/markdownPipeline/serializer */ @@ -36,6 +40,7 @@ import remarkFrontmatter from "remark-frontmatter"; import type { Root } from "mdast"; import { remarkCustomInline, remarkDetailsBlock, remarkWikiLinks, tocToMarkdown } from "./plugins"; import { handleImage, handleLink, blankLinesJoin } from "./serializerHandlers"; +import { listInterruptJoin } from "./listInterruptJoin"; import type { MarkdownPipelineOptions } from "./types"; import { parseMarkdownToMdast } from "./parser"; import { applyCosmeticPass } from "./serializerCosmetics"; @@ -80,7 +85,9 @@ function buildSerializer() { delete: handleDelete, ...tocToMarkdown.handlers, } as Record, - join: [blankLinesJoin], // re-emit captured blank-line runs (ADR-1a) + // Joins are consulted last-first: listInterruptJoin can raise a captured + // blank-line run (ADR-1a) that CommonMark would read as paragraph text. + join: [blankLinesJoin, listInterruptJoin], } as Parameters[0]) .use(remarkGfm, { singleTilde: false, // Match parser config diff --git a/src/utils/markdownPipeline/serializerHandlers.ts b/src/utils/markdownPipeline/serializerHandlers.ts index efbe3b673..d11a7f251 100644 --- a/src/utils/markdownPipeline/serializerHandlers.ts +++ b/src/utils/markdownPipeline/serializerHandlers.ts @@ -236,7 +236,7 @@ export const handleLink = Object.assign(linkHandler, { */ export function blankLinesJoin( _left: unknown, - right: { data?: { blankLinesBefore?: unknown } }, + right: { data?: { blankLinesBefore?: unknown } | undefined }, ): number | undefined { const n = right?.data?.blankLinesBefore; // Only a finite integer in the captured range is a valid separator count; From ae22d0e985a43e0313ba0093aff2b419531bffd5 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 13:08:42 +0800 Subject: [PATCH 04/11] fix(editor): keep undo from being rewritten into a corrupt history Tiptap core's clearDocument plugin turns a document emptied by select-all-and-delete back into a plain paragraph. Its test is "the old selection covered everything and the new document is empty", and an undo taken with the text fully selected passes it whenever it leaves an empty list item or heading behind, so the plugin lifted that item in an appended transaction. prosemirror-history files a transaction appended to an undo in the redo branch without remapping what is left in the undo branch. The first undo therefore showed a document that never existed (a paragraph where the empty list item was), and the next undo replayed steps recorded for a document that no longer existed: `RangeError: Position 6 out of range`. The soak's editing fuzz found it (#1407, seed 4). A new undoIntegrity extension tags every history transaction with the plugin's own `preventClearDocument` opt-out, in the extension dispatch hook that both the undo/redo commands and prosemirror-history's beforeinput handler go through. Select-all-and-delete keeps its normalization, and a test pins that. --- src/plugins/undoIntegrity/tiptap.test.ts | 100 ++++++++++++++++++ src/plugins/undoIntegrity/tiptap.ts | 44 ++++++++ src/services/assembly/compositionOrder.ts | 1 + .../assembly/tiptapExtensions.test.ts | 9 +- src/services/assembly/tiptapExtensions.ts | 4 + 5 files changed, 154 insertions(+), 4 deletions(-) create mode 100644 src/plugins/undoIntegrity/tiptap.test.ts create mode 100644 src/plugins/undoIntegrity/tiptap.ts diff --git a/src/plugins/undoIntegrity/tiptap.test.ts b/src/plugins/undoIntegrity/tiptap.test.ts new file mode 100644 index 000000000..73859eb5f --- /dev/null +++ b/src/plugins/undoIntegrity/tiptap.test.ts @@ -0,0 +1,100 @@ +/** + * Undo restores the document it recorded, and the undo history stays usable. + * + * Tiptap's core `clearDocument` plugin turns a document emptied by + * "select everything, then delete" back into a plain paragraph. It keys on the + * OLD selection covering the whole document and the NEW document being empty, + * so an UNDO taken with the text fully selected qualifies too, whenever it + * leaves an empty list item or heading behind. The plugin then lifted that + * item in an appended transaction. + * + * prosemirror-history files a transaction appended to an undo in the REDO + * branch without remapping what remains in the undo branch. The next undo + * applied steps recorded for a document that no longer existed and threw + * `RangeError: Position 6 out of range` — found by the soak's editing fuzz + * (#1407, seed 4). Before it threw, the first undo had already produced a + * document that never existed: a paragraph where the empty list item was. + */ +import { afterEach, describe, expect, it } from "vitest"; +import { Selection } from "@tiptap/pm/state"; +import { createTypingSession, type TypingSession } from "@/test/typingHarness"; + +let session: TypingSession | null = null; + +afterEach(() => { + session?.destroy(); + session = null; +}); + +/** Select from the first to the last text position, as a drag or Shift-click does. */ +function selectAllText(s: TypingSession): void { + const { doc } = s.editor.state; + s.select(Selection.atStart(doc).from, Selection.atEnd(doc).to); +} + +const blockTypes = (s: TypingSession): string[] => { + const types: string[] = []; + s.editor.state.doc.descendants((node) => { + if (node.isBlock) types.push(node.type.name); + }); + return types; +}; + +/** Undo until the history is exhausted; returns how many undos ran. */ +function undoAll(s: TypingSession): number { + let count = 0; + while (s.undo()) { + count += 1; + if (count > 200) throw new Error("undo did not terminate"); + } + return count; +} + +describe("undo with the whole document selected", () => { + // Seed 4's minimized trace. + function typeListThenSelectAll(): TypingSession { + const s = createTypingSession({ markdown: "" }); + s.type("* "); + s.type("wordword"); + s.press("Enter"); + s.type("word"); + selectAllText(s); + return s; + } + + it("restores the empty list item the typing started from", () => { + session = typeListThenSelectAll(); + session.undo(); + expect(blockTypes(session)).toEqual(["bulletList", "listItem", "paragraph", "paragraph"]); + }); + + it("keeps undoing back to the empty document without throwing (seed 4)", () => { + session = typeListThenSelectAll(); + expect(() => undoAll(session as TypingSession)).not.toThrow(); + expect(session.editor.state.doc.textContent).toBe(""); + expect(blockTypes(session)).not.toContain("bulletList"); + }); + + it("redoes everything it undid", () => { + session = typeListThenSelectAll(); + const before = session.editor.state.doc.toJSON(); + undoAll(session); + let redone = 0; + expect(() => { + while (session?.redo()) redone += 1; + }).not.toThrow(); + expect(redone).toBeGreaterThan(0); + expect(session.editor.state.doc.toJSON()).toEqual(before); + }); +}); + +// The guard is scoped to history transactions: the case clearDocument exists +// for must keep working. +describe("select everything, then delete", () => { + it("still turns an emptied heading back into a paragraph", () => { + session = createTypingSession({ markdown: "# Title\n" }); + selectAllText(session); + session.press("Backspace"); + expect(blockTypes(session)).toEqual(["paragraph"]); + }); +}); diff --git a/src/plugins/undoIntegrity/tiptap.ts b/src/plugins/undoIntegrity/tiptap.ts new file mode 100644 index 000000000..f65fa9fc0 --- /dev/null +++ b/src/plugins/undoIntegrity/tiptap.ts @@ -0,0 +1,44 @@ +/** + * Undo integrity — keep document normalizers from rewriting an undo or redo. + * + * Purpose: an undo must restore the document it recorded, and leave the rest + * of the history applicable to the result. + * + * Tiptap core's `clearDocument` plugin (the `keymap` core extension) turns a + * document emptied by select-all-and-delete back into a plain paragraph. Its + * test is "the old selection covered everything and the new document is + * empty", which an UNDO taken with the text fully selected also passes whenever + * it leaves an empty list item or heading behind — so it lifted that item in an + * appended transaction. prosemirror-history files a transaction appended to an + * undo in the redo branch WITHOUT remapping the undo branch, so the next undo + * replayed steps recorded for a document that no longer existed: + * `RangeError: Position 6 out of range` (#1407 soak, editing fuzz seed 4). + * + * Key decisions: + * - Opt out with the plugin's own `preventClearDocument` meta rather than + * disabling the core extension, which also owns Backspace/Delete/Enter. + * - Tag in the extension `dispatchTransaction` hook, which every view + * dispatch passes through — the undo/redo commands and prosemirror-history's + * own `beforeinput` handler alike — before the state applies it. + * - Only history transactions are tagged; select-all-and-delete keeps its + * normalization. + * + * @coordinates-with services/assembly/tiptapExtensions.ts — registers it + * @module plugins/undoIntegrity/tiptap + */ +import { Extension } from "@tiptap/core"; +import { isHistoryTransaction } from "@tiptap/pm/history"; + +/** The meta Tiptap's `clearDocument` plugin honours as an opt-out. */ +const PREVENT_CLEAR_DOCUMENT = "preventClearDocument"; + +export const undoIntegrityExtension = Extension.create({ + name: "undoIntegrity", + + dispatchTransaction({ transaction, next }) { + if (isHistoryTransaction(transaction)) { + transaction.setMeta(PREVENT_CLEAR_DOCUMENT, true); + } + next(transaction); + }, +}); diff --git a/src/services/assembly/compositionOrder.ts b/src/services/assembly/compositionOrder.ts index e91bd1cf4..156d34e3f 100644 --- a/src/services/assembly/compositionOrder.ts +++ b/src/services/assembly/compositionOrder.ts @@ -21,6 +21,7 @@ */ export const WYSIWYG_COMPOSITION_ORDER: readonly string[] = [ "starterKit", + "undoIntegrity", "link", "bold", "italic", diff --git a/src/services/assembly/tiptapExtensions.test.ts b/src/services/assembly/tiptapExtensions.test.ts index 6b2f0a240..2440cd76d 100644 --- a/src/services/assembly/tiptapExtensions.test.ts +++ b/src/services/assembly/tiptapExtensions.test.ts @@ -9,10 +9,11 @@ import { WYSIWYG_COMPOSITION_ORDER } from "./compositionOrder"; describe("WI-3.4 — WYSIWYG composition order", () => { // 79 since audit 20260906 F5 added `safeBlockSplit` — Enter on a cross-block - // selection, which StarterKit's splitBlock throws on. - it("has 79 unique canonical entries", () => { - expect(WYSIWYG_COMPOSITION_ORDER.length).toBe(79); - expect(new Set(WYSIWYG_COMPOSITION_ORDER).size).toBe(79); + // selection, which StarterKit's splitBlock throws on. 80 since `undoIntegrity` + // stopped clearDocument rewriting an undo (#1407 soak, fuzz seed 4). + it("has 80 unique canonical entries", () => { + expect(WYSIWYG_COMPOSITION_ORDER.length).toBe(80); + expect(new Set(WYSIWYG_COMPOSITION_ORDER).size).toBe(80); }); it("resolves to exactly the canonical order when a tab is known (lint present)", () => { diff --git a/src/services/assembly/tiptapExtensions.ts b/src/services/assembly/tiptapExtensions.ts index 6e2482fee..dc072a47e 100644 --- a/src/services/assembly/tiptapExtensions.ts +++ b/src/services/assembly/tiptapExtensions.ts @@ -61,6 +61,7 @@ import { import { useSettingsStore } from "@/stores/settingsStore"; import { compositionGuardExtension } from "@/plugins/compositionGuard/tiptap"; import { blankLinesGuardExtension } from "@/plugins/blankLinesGuard/tiptap"; +import { undoIntegrityExtension } from "@/plugins/undoIntegrity/tiptap"; import { focusModeExtension } from "@/plugins/focusMode/tiptap"; import { focusModeHostOptions, typewriterModeHostOptions } from "./uiToggleOptions"; import { typewriterModeExtension } from "@/plugins/typewriterMode/tiptap"; @@ -160,6 +161,9 @@ export function buildExtensionList(config: TiptapExtensionConfig = {}): Extensio newGroupDelay: 500, }, }), + // Keeps Tiptap's clearDocument normalizer from rewriting an undo/redo, + // which corrupted the remaining undo history (plugins/undoIntegrity). + undoIntegrityExtension, vmarkLinkExtension, // CJK-aware bold/italic (replaces StarterKit defaults) CJKBold, From 51111480124a8a7da2578278f2757305b1092ecf Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 13:19:15 +0800 Subject: [PATCH 05/11] fix(soak): run the documented fuzz seed and refuse a silent empty input The weekly soak never ran its documented seed. soak.yml passed `FUZZ_SEED: ${{ inputs.fuzz_seed }}`, which is the EMPTY STRING on a scheduled run, and editingFuzz.test.ts read `Number(process.env.FUZZ_SEED ?? "20260805")`. `??` falls back on null/undefined only and `Number("")` is 0, so every scheduled run fuzzed seed 0. At 20260805, and at seeds 3 to 7, the fuzz fails on the editor bugs fixed in the preceding commits (#1407 found the mechanism). Fixed as a class, in one pass: - soak.yml falls back to 20260805 in the expression, and runs the fuzz with the verbose reporter, because the default reporter prints no test names for a passing file and the name is what carries the runs and seed. - src/test/envInteger.ts reads an integer knob strictly: unset means the default, anything set that is not an integer literal throws with the name and value. All four numeric environment reads in the repo use it: FUZZ_RUNS, FUZZ_SEED, FAST_PATH_SEED, and PATHOLOGICAL_SCALE, where an empty value used to shrink every pathological input to the 4-character floor. - Every other workflow input handed to a step was checked. release-smoke.yml (twice) and update-homebrew.yml already handle an empty value on purpose, and now say so with a reasoned `# input-empty-ok:` marker. release.yml pasted its version input into the shell body; it now goes through env like the rest. Two gates keep it fixed, both mutation-checked by reverting a fix and watching them fail: - check-workflow-input-fallbacks.test.mjs parses every workflow as YAML and fails on an env value that reads an input with neither a fallback nor a reasoned marker, and on any input interpolated into a run body. - check-numeric-env-reads.test.mjs walks a TypeScript AST over the repo and fails on Number/parseInt/parseFloat/unary plus applied to process.env. --- .github/workflows/release-smoke.yml | 4 +- .github/workflows/release.yml | 5 +- .github/workflows/soak.yml | 16 +- .github/workflows/update-homebrew.yml | 2 +- scripts/check-numeric-env-reads.test.mjs | 133 +++++++++++++++ .../check-workflow-input-fallbacks.test.mjs | 156 ++++++++++++++++++ src/test/editingFuzz.test.ts | 11 +- src/test/envInteger.test.ts | 63 +++++++ src/test/envInteger.ts | 46 ++++++ .../pathological/pathological.test.ts | 2 +- .../pathological/pathologicalCases.ts | 8 +- .../parser/fastPaths/inlineFastPaths.test.ts | 3 +- 12 files changed, 435 insertions(+), 14 deletions(-) create mode 100644 scripts/check-numeric-env-reads.test.mjs create mode 100644 scripts/check-workflow-input-fallbacks.test.mjs create mode 100644 src/test/envInteger.test.ts create mode 100644 src/test/envInteger.ts diff --git a/.github/workflows/release-smoke.yml b/.github/workflows/release-smoke.yml index 2e731b04a..66da61465 100644 --- a/.github/workflows/release-smoke.yml +++ b/.github/workflows/release-smoke.yml @@ -41,7 +41,7 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} # Event-supplied values go through env, never interpolated into the # shell body (workflow-injection safety). - INPUT_TAG: ${{ github.event.inputs.tag }} + INPUT_TAG: ${{ github.event.inputs.tag }} # input-empty-ok: empty on the release event; the step falls back to the release tag, then the latest release, and rejects anything not v* RELEASE_TAG: ${{ github.event.release.tag_name }} run: | TAG="${INPUT_TAG:-$RELEASE_TAG}" @@ -236,7 +236,7 @@ jobs: GH_REPO: ${{ github.repository }} # Event-supplied values go through env, never interpolated into the # shell body (workflow-injection safety). - INPUT_TAG: ${{ github.event.inputs.tag }} + INPUT_TAG: ${{ github.event.inputs.tag }} # input-empty-ok: empty on the release event; the step falls back to the release tag, then the latest release, and rejects anything not v* RELEASE_TAG: ${{ github.event.release.tag_name }} run: | TAG="${INPUT_TAG:-$RELEASE_TAG}" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 5910b9958..3facca227 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -35,9 +35,12 @@ jobs: - name: Get version id: get-version + env: + # Through env, never pasted into the shell body (workflow-injection + # safety, and check-workflow-input-fallbacks.test.mjs). + INPUT_VERSION: ${{ github.event.inputs.version }} # input-empty-ok: empty on a tag push, where it is not read; a dispatch rejects anything not vX.Y.Z below run: | if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then - INPUT_VERSION="${{ github.event.inputs.version }}" # Strict format: vX.Y.Z (optionally with -prerelease or +build). # Manual-dispatch typos otherwise create bogus releases that pass # the rest of the pipeline (semver validator on latest.json runs diff --git a/.github/workflows/soak.yml b/.github/workflows/soak.yml index 3f5792642..4bea3436d 100644 --- a/.github/workflows/soak.yml +++ b/.github/workflows/soak.yml @@ -24,7 +24,7 @@ on: description: "fast-check runs for the editing fuzz" default: "500" fuzz_seed: - description: "fast-check seed (empty = keep CI default)" + description: "fast-check seed (empty = the CI default, 20260805)" default: "" permissions: @@ -47,7 +47,10 @@ jobs: include: - name: fuzz label: Deep editing-op fuzz (scaled) - command: pnpm vitest run src/test/editingFuzz.test.ts + # verbose: the test NAME carries the runs and seed, and the default + # reporter prints no names for a passing file, so a green log + # would not say which seed actually ran. + command: pnpm vitest run src/test/editingFuzz.test.ts --reporter=verbose - name: pathological label: Full-size pathological inputs (killable child) command: pnpm vitest run src/utils/markdownPipeline/__tests__/pathological/ @@ -69,8 +72,15 @@ jobs: - name: ${{ matrix.label }} env: # Only the fuzz reads these; the others ignore them. + # + # `inputs.*` is the EMPTY STRING on a scheduled run, so every input + # needs its fallback here. FUZZ_SEED had none, and the test turned "" + # into seed 0: the weekly soak never ran its documented seed, which + # finds three editor bugs seed 0 does not (#1407). The test now + # refuses a non-integer, and check-workflow-input-fallbacks.test.mjs + # refuses an input without a fallback. FUZZ_RUNS: ${{ inputs.fuzz_runs || '500' }} - FUZZ_SEED: ${{ inputs.fuzz_seed }} + FUZZ_SEED: ${{ inputs.fuzz_seed || '20260805' }} PATHOLOGICAL_SCALE: "8" run: ${{ matrix.command }} diff --git a/.github/workflows/update-homebrew.yml b/.github/workflows/update-homebrew.yml index 4ec42eca3..5b1289e84 100644 --- a/.github/workflows/update-homebrew.yml +++ b/.github/workflows/update-homebrew.yml @@ -28,7 +28,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 25 env: - VERSION: ${{ github.event.inputs.version }} + VERSION: ${{ github.event.inputs.version }} # input-empty-ok: required dispatch input; the first step rejects anything not X.Y.Z REPO: xiaolai/vmark steps: - name: Validate version input diff --git a/scripts/check-numeric-env-reads.test.mjs b/scripts/check-numeric-env-reads.test.mjs new file mode 100644 index 000000000..5bf05d11d --- /dev/null +++ b/scripts/check-numeric-env-reads.test.mjs @@ -0,0 +1,133 @@ +/** + * An integer read from the environment goes through `readIntegerEnv`, never + * through `Number(process.env…)` / `parseInt(process.env…)`. + * + * `Number(process.env.FUZZ_SEED ?? "20260805")` turned a set-but-empty variable + * into 0 and a typo into NaN, with no error. The weekly soak ran its editing + * fuzz at seed 0 on every scheduled run that way (#1407), and the pathological + * suite would have shrunk to trivial inputs on an empty `PATHOLOGICAL_SCALE`. + * `src/test/envInteger.ts` refuses both; this keeps the old shape from being + * written again next to it. + * + * It walks a TypeScript AST rather than grepping, so an explanation of the + * defect in a comment or a string — like the one above — is prose, not a + * finding. Only files that mention `process.env` are parsed. Tracked and + * untracked-not-ignored files are listed by `git ls-files`, so generated and + * ignored trees cannot contribute. + * + * @coordinates-with src/test/envInteger.ts — the reader this requires + * @coordinates-with scripts/check-workflow-input-fallbacks.test.mjs — the + * workflow half: inputs never reach a step as a silent empty string + * @module scripts/check-numeric-env-reads.test + */ +import { describe, expect, it } from "vitest"; +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import ts from "typescript"; + +const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const CONVERTERS = new Set(["Number", "parseInt", "parseFloat", "Number.parseInt", "Number.parseFloat"]); + +/** `a.b.c` for a chain of identifiers, or null for anything else. */ +function dottedName(node) { + if (ts.isIdentifier(node)) return node.text; + if (ts.isPropertyAccessExpression(node)) { + const left = dottedName(node.expression); + return left === null ? null : `${left}.${node.name.text}`; + } + return null; +} + +/** Whether `node` reads `process.env.X` / `process.env["X"]`, possibly with a `??`/`||` default. */ +function readsProcessEnv(node) { + let current = node; + while (ts.isParenthesizedExpression(current)) current = current.expression; + if ( + ts.isBinaryExpression(current) && + (current.operatorToken.kind === ts.SyntaxKind.QuestionQuestionToken || + current.operatorToken.kind === ts.SyntaxKind.BarBarToken) + ) { + return readsProcessEnv(current.left); + } + if (ts.isPropertyAccessExpression(current) || ts.isElementAccessExpression(current)) { + const owner = dottedName(current.expression); + return owner === "process.env" || owner === "globalThis.process.env"; + } + return false; +} + +/** `file:line` for every numeric conversion applied directly to an environment read. */ +function numericEnvReads(source, file) { + if (!source.includes("process.env")) return []; + const sourceFile = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + const out = []; + const visit = (node) => { + const converted = + (ts.isCallExpression(node) && + CONVERTERS.has(dottedName(node.expression) ?? "") && + node.arguments.length > 0 && + readsProcessEnv(node.arguments[0])) || + (ts.isPrefixUnaryExpression(node) && + node.operator === ts.SyntaxKind.PlusToken && + readsProcessEnv(node.operand)); + if (converted) { + out.push(`${file}:${sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1}`); + } + ts.forEachChild(node, visit); + }; + visit(sourceFile); + return out; +} + +const files = execFileSync( + "git", + ["ls-files", "--cached", "--others", "--exclude-standard", "*.ts", "*.tsx", "*.mts", "*.cts", "*.js", "*.mjs", "*.cjs"], + { cwd: REPO, encoding: "utf8" }, +) + .split("\n") + .filter(Boolean); + +describe("numeric environment reads", () => { + it("scans the repository (guards against a silently empty sweep)", () => { + expect(files.length).toBeGreaterThan(1000); + expect(files).toContain("src/test/editingFuzz.test.ts"); + }); + + it("go through readIntegerEnv", () => { + const offenders = files.flatMap((file) => { + let source; + try { + source = readFileSync(path.join(REPO, file), "utf8"); + } catch { + return []; // listed by git but deleted in the working tree + } + return numericEnvReads(source, file); + }); + expect(offenders).toEqual([]); + }); +}); + +describe("SELF-TEST: the detector", () => { + it.each([ + 'const SEED = Number(process.env.FUZZ_SEED ?? "20260805");', + "const n = parseInt(process.env.N, 10);", + "const s = Number.parseFloat( (process.env.SCALE) );", + 'const k = Number(process.env["KNOB"] || "3");', + "const p = +process.env.PORT;", + "const g = Number(globalThis.process.env.G);", + ])("flags %s", (line) => { + expect(numericEnvReads(line, "x.ts")).toEqual(["x.ts:1"]); + }); + + it.each([ + 'const SEED = readIntegerEnv("FUZZ_SEED", 20260805);', + 'const base = process.env.VMARK_CHANGED_BASE ?? "origin/main";', + "const n = Number(settings.count);", + '// the old shape: Number(process.env.FUZZ_SEED ?? "20260805")', + 'const doc = "Number(process.env.X)";', + ])("leaves %s alone", (line) => { + expect(numericEnvReads(line, "x.ts")).toEqual([]); + }); +}); diff --git a/scripts/check-workflow-input-fallbacks.test.mjs b/scripts/check-workflow-input-fallbacks.test.mjs new file mode 100644 index 000000000..9c5e5ef1a --- /dev/null +++ b/scripts/check-workflow-input-fallbacks.test.mjs @@ -0,0 +1,156 @@ +/** + * A workflow input handed to a step must never arrive as a silent empty string. + * + * `inputs.*` (and `github.event.inputs.*`) is the EMPTY STRING whenever the run + * was not a dispatch that supplied it: every scheduled run, every push, every + * release event, and every dispatch that left an optional field blank. A + * consumer written for "unset" does not see "unset". The weekly soak passed + * `FUZZ_SEED: ${{ inputs.fuzz_seed }}` to a test that read + * `Number(process.env.FUZZ_SEED ?? "20260805")`: `??` does not fire on "", and + * `Number("")` is 0. Every scheduled soak ran seed 0 instead of the documented + * seed, and the three editor bugs the documented seed finds stayed hidden + * (#1407). The step was green the whole time. + * + * So this asserts, for every workflow: + * + * 1. An `env:` value that reads an input either supplies the fallback in the + * expression — `${{ inputs.x || 'default' }}` — or carries a trailing + * `# input-empty-ok: ` comment saying why its consumer handles "" + * on purpose. The reason is required: a bare marker is a mute button. + * 2. No `run:` body interpolates an input at all. There the value is pasted + * into the shell before it runs, which both skips this check and is the + * workflow-injection shape the repo's workflows already route through + * `env:` to avoid. + * + * `with:` is out of scope deliberately: actions read inputs through + * `core.getInput`, which already treats "" as not provided. + * + * Workflows are discovered, not listed, so a new one is covered on creation. + * + * @coordinates-with .github/workflows/*.yml + * @coordinates-with src/test/envInteger.ts — the consumer-side half: an + * integer knob that is present but not an integer fails instead of reading 0 + * @module scripts/check-workflow-input-fallbacks.test + */ +import { describe, expect, it } from "vitest"; +import { readdirSync, readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { isMap, isPair, isScalar, parseDocument, visit } from "yaml"; + +const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const DIR = path.join(REPO, ".github/workflows"); + +/** An `inputs.` read inside one `${{ … }}` expression. */ +const INPUT_READ = /\b(?:github\.event\.)?inputs\.[A-Za-z_][A-Za-z0-9_-]*/; +const EXPRESSION = /\$\{\{([\s\S]*?)\}\}/g; +const MARKER = /^\s*input-empty-ok:\s*(\S.*)?$/; + +/** Every `${{ … }}` body in `text` that reads an input. */ +function inputExpressions(text) { + return [...text.matchAll(EXPRESSION)].map((m) => m[1]).filter((body) => INPUT_READ.test(body)); +} + +/** Whether an expression supplies a fallback after the input it reads. */ +function hasFallback(body) { + const read = INPUT_READ.exec(body); + return read !== null && /\|\|\s*\S/.test(body.slice(read.index + read[0].length)); +} + +/** + * Findings for one workflow source. Exported shape is `{ where, problem }` so + * the self-tests below can assert against synthetic workflows. + */ +function findings(source, file) { + const doc = parseDocument(source); + const out = []; + visit(doc, { + Pair(_key, pair) { + if (!isScalar(pair.key)) return; + const key = String(pair.key.value); + + if (key === "env" && isMap(pair.value)) { + for (const entry of pair.value.items) { + if (!isPair(entry) || !isScalar(entry.value) || typeof entry.value.value !== "string") continue; + const name = String(isScalar(entry.key) ? entry.key.value : "?"); + for (const body of inputExpressions(entry.value.value)) { + if (hasFallback(body)) continue; + const marker = MARKER.exec(entry.value.comment ?? ""); + if (marker && marker[1]) continue; + out.push({ + where: `${file} env ${name}`, + problem: marker + ? "input-empty-ok marker has no reason" + : `reads \`${body.trim()}\` with no fallback and no input-empty-ok marker`, + }); + } + } + } + + if (key === "run" && isScalar(pair.value) && typeof pair.value.value === "string") { + for (const body of inputExpressions(pair.value.value)) { + out.push({ + where: `${file} run`, + problem: `interpolates \`${body.trim()}\` into the shell; pass it through env:`, + }); + } + } + }, + }); + return out; +} + +const workflows = readdirSync(DIR) + .filter((f) => f.endsWith(".yml") || f.endsWith(".yaml")) + .map((f) => ({ file: f, source: readFileSync(path.join(DIR, f), "utf8") })); + +describe("workflow inputs reach steps with a fallback", () => { + it("finds the workflows (guards against a silently empty sweep)", () => { + expect(workflows.length).toBeGreaterThan(5); + expect(workflows.map((w) => w.file)).toContain("soak.yml"); + }); + + it("every input read in env: has a fallback or a reasoned marker, and none is pasted into run:", () => { + const all = workflows.flatMap(({ file, source }) => findings(source, file)); + expect(all).toEqual([]); + }); +}); + +// The checker is only worth what it can still catch. +describe("SELF-TEST: the checker", () => { + const wf = (env, run = "echo ok") => + `on: workflow_dispatch\njobs:\n a:\n runs-on: ubuntu-latest\n steps:\n - env:\n${env}\n run: ${run}\n`; + + it("flags the soak's original line", () => { + expect(findings(wf(" FUZZ_SEED: ${{ inputs.fuzz_seed }}"), "t.yml")).toHaveLength(1); + }); + + it("flags github.event.inputs as well", () => { + expect(findings(wf(" TAG: ${{ github.event.inputs.tag }}"), "t.yml")).toHaveLength(1); + }); + + it("accepts a fallback in the expression", () => { + expect(findings(wf(" FUZZ_SEED: ${{ inputs.fuzz_seed || '20260805' }}"), "t.yml")).toEqual([]); + }); + + it("does not count a fallback that precedes the input", () => { + expect(findings(wf(" X: ${{ github.ref || inputs.x }}"), "t.yml")).toHaveLength(1); + }); + + it("accepts a reasoned marker and refuses a bare one", () => { + expect( + findings(wf(" TAG: ${{ inputs.tag }} # input-empty-ok: the step defaults it"), "t.yml"), + ).toEqual([]); + const bare = findings(wf(" TAG: ${{ inputs.tag }} # input-empty-ok:"), "t.yml"); + expect(bare.map((f) => f.problem)).toEqual(["input-empty-ok marker has no reason"]); + }); + + it("flags an input pasted into a run body, even with a fallback", () => { + const run = `|\n echo "\${{ inputs.version || 'v0' }}"`; + expect(findings(wf(" A: plain", run), "t.yml")).toHaveLength(1); + }); + + it("ignores env values that read no input", () => { + expect(findings(wf(" GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}"), "t.yml")).toEqual([]); + }); +}); diff --git a/src/test/editingFuzz.test.ts b/src/test/editingFuzz.test.ts index dbfa39442..799f9ed73 100644 --- a/src/test/editingFuzz.test.ts +++ b/src/test/editingFuzz.test.ts @@ -25,7 +25,9 @@ * Budget: 25 runs × ≤40 ops (~1k transactions) — the roundtrip property * suite flaked at 200–300 CPU-bound runs under worker contention, so this * stays deliberately small in the PR tier; FUZZ_RUNS scales it in the soak. - * Seed fixed for CI determinism; override with FUZZ_SEED to explore. + * Seed fixed for CI determinism; override with FUZZ_SEED to explore. Both are + * read with readIntegerEnv, so a value that is set but not an integer fails + * the file instead of running seed 0. * * Declared exclusion (measured): '[' is not in the text pools — autoPair's * bracket pairing plus link-reference parsing has a known escape-growth @@ -42,6 +44,7 @@ import type { Node as PmNode } from "@tiptap/pm/model"; import { TextSelection } from "@tiptap/pm/state"; import { parseMarkdown, serializeMarkdown } from "@/utils/markdownPipeline/adapter"; import { createTypingSession, type TypingSession } from "./typingHarness"; +import { readIntegerEnv } from "./envInteger"; // ── op vocabulary ───────────────────────────────────────────────────────── const TEXT_POOL = [ @@ -249,8 +252,10 @@ function fingerprint(node: PmNode): unknown { } const fp = (n: PmNode) => JSON.stringify(fingerprint(n)); -const RUNS = Number(process.env.FUZZ_RUNS ?? "25"); -const SEED = Number(process.env.FUZZ_SEED ?? "20260805"); +// Read strictly: a set-but-empty FUZZ_SEED used to become seed 0 silently, and +// the weekly soak ran seed 0 for months (#1407). The test name prints both. +const RUNS = readIntegerEnv("FUZZ_RUNS", 25, { min: 1 }); +const SEED = readIntegerEnv("FUZZ_SEED", 20260805); describe("editing-op fuzz (production stack)", () => { it(`random op traces preserve every invariant (${RUNS} runs, seed ${SEED})`, () => { diff --git a/src/test/envInteger.test.ts b/src/test/envInteger.test.ts new file mode 100644 index 000000000..ecba0e9f1 --- /dev/null +++ b/src/test/envInteger.test.ts @@ -0,0 +1,63 @@ +// @vitest-environment node +/** + * An integer knob read from the environment is the default when unset, and an + * error — never 0 — when set to anything that is not an integer. + * + * The weekly soak ran its editing fuzz at seed 0 on every scheduled run: the + * workflow passed `FUZZ_SEED: ${{ inputs.fuzz_seed }}`, which is the EMPTY + * STRING when no dispatch inputs exist, and the test read + * `Number(process.env.FUZZ_SEED ?? "20260805")`. `??` falls back on + * null/undefined only, and `Number("") === 0`. At the documented seed the fuzz + * failed on three real editor bugs that seed 0 never reached (#1407). + */ +import { afterEach, describe, expect, it } from "vitest"; +import { readIntegerEnv } from "./envInteger"; + +const NAME = "VMARK_TEST_INTEGER_KNOB"; + +afterEach(() => { + delete process.env[NAME]; +}); + +describe("readIntegerEnv", () => { + it("returns the fallback when the variable is unset", () => { + expect(readIntegerEnv(NAME, 20260805)).toBe(20260805); + }); + + it.each([ + ["500", 500], + ["0", 0], + ["-7", -7], + ["20260805", 20260805], + ])("reads %j as %i", (raw, expected) => { + process.env[NAME] = raw; + expect(readIntegerEnv(NAME, 1)).toBe(expected); + }); + + // The defect: present but empty must not quietly become 0. + it("refuses an empty value, naming the variable", () => { + process.env[NAME] = ""; + expect(() => readIntegerEnv(NAME, 20260805)).toThrow(/VMARK_TEST_INTEGER_KNOB/); + expect(() => readIntegerEnv(NAME, 20260805)).toThrow(/""/); + }); + + it.each(["abc", "1.5", "1e3", "0x10", " 5", "5 ", "+5", "NaN", "Infinity", "٣"])( + "refuses %j", + (raw) => { + process.env[NAME] = raw; + expect(() => readIntegerEnv(NAME, 1)).toThrow(/must be an integer/); + }, + ); + + it("refuses an integer too large to hold exactly", () => { + process.env[NAME] = "9007199254740993"; + expect(() => readIntegerEnv(NAME, 1)).toThrow(/must be an integer/); + }); + + it("enforces a minimum", () => { + process.env[NAME] = "0"; + expect(() => readIntegerEnv(NAME, 25, { min: 1 })).toThrow(/at least 1/); + process.env[NAME] = "1"; + expect(readIntegerEnv(NAME, 25, { min: 1 })).toBe(1); + }); +}); diff --git a/src/test/envInteger.ts b/src/test/envInteger.ts new file mode 100644 index 000000000..14445d9ec --- /dev/null +++ b/src/test/envInteger.ts @@ -0,0 +1,46 @@ +/** + * Purpose: read an integer tuning knob (a fuzz seed, a run count, a scale) + * from the environment without ever turning a bad value into 0. + * + * `Number(process.env.X ?? "default")` has two silent failure modes, and the + * weekly soak hit the first: `??` falls back on null/undefined only, so a + * variable that is SET but EMPTY — which is what a GitHub Actions + * `${{ inputs.x }}` expands to on a scheduled run — reaches `Number("")` and + * becomes 0. The soak's editing fuzz ran seed 0 instead of its documented seed + * on every scheduled run, and missed three editor bugs (#1407). The second mode + * is `Number("abc")`: NaN, which most consumers then clamp or ignore. + * + * So: unset means "use the default"; set means "this exact integer", and + * anything else throws with the variable's name and the value it held. + * + * @coordinates-with scripts/check-workflow-input-fallbacks.test.mjs — the + * workflow-side half: no input reaches a step as a silent empty string + * @module test/envInteger + */ + +/** An optional-sign run of ASCII digits and nothing else. */ +const INTEGER_LITERAL = /^-?[0-9]+$/; + +/** + * The integer in `process.env[name]`, or `fallback` when it is unset. + * + * @throws when the variable is set to anything but an integer literal that + * fits exactly in a double (empty, whitespace, `1.5`, `1e3`, `0x10`, …), or + * to one below `options.min`. + */ +export function readIntegerEnv(name: string, fallback: number, options: { min?: number } = {}): number { + const raw = process.env[name]; + if (raw === undefined) return fallback; + + const value = Number(raw); + if (!INTEGER_LITERAL.test(raw) || !Number.isSafeInteger(value)) { + throw new Error( + `${name} must be an integer, got ${JSON.stringify(raw)}. ` + + `Unset it to use the default (${fallback}).`, + ); + } + if (options.min !== undefined && value < options.min) { + throw new Error(`${name} must be at least ${options.min}, got ${value}.`); + } + return value; +} diff --git a/src/utils/markdownPipeline/__tests__/pathological/pathological.test.ts b/src/utils/markdownPipeline/__tests__/pathological/pathological.test.ts index 4ef1d498f..40b5443b2 100644 --- a/src/utils/markdownPipeline/__tests__/pathological/pathological.test.ts +++ b/src/utils/markdownPipeline/__tests__/pathological/pathological.test.ts @@ -117,7 +117,7 @@ function waitForNoRunners(budgetMs: number): string[] { } describe("pathological inputs (killable child process)", () => { - it("every pathological class parses and serializes within the wall ceiling", () => { + it(`every pathological class parses and serializes within the wall ceiling (scale ${pathologicalScale()})`, () => { const { res, lines } = runChild({}, WALL_CEILING_MS); const started = lines.filter((l) => l.starting && l.name).map((l) => l.name); diff --git a/src/utils/markdownPipeline/__tests__/pathological/pathologicalCases.ts b/src/utils/markdownPipeline/__tests__/pathological/pathologicalCases.ts index 7d8c402d4..55e902a06 100644 --- a/src/utils/markdownPipeline/__tests__/pathological/pathologicalCases.ts +++ b/src/utils/markdownPipeline/__tests__/pathological/pathologicalCases.ts @@ -15,6 +15,7 @@ * @coordinates-with pathological.test.ts — the killing parent * @module utils/markdownPipeline/__tests__/pathological/pathologicalCases */ +import { readIntegerEnv } from "@/test/envInteger"; export interface PathologicalCase { name: string; @@ -31,9 +32,12 @@ export interface PathologicalCase { } /** The scale a run uses: `PATHOLOGICAL_SCALE`, read in ONE place so the child - * that generates the inputs and the parent that judges them cannot disagree. */ + * that generates the inputs and the parent that judges them cannot disagree. + * Read strictly: `Number("")` is 0, and `pathologicalCases` clamps every size + * to at least 4, so an empty value used to shrink the soak to trivial inputs + * that pass without testing anything. */ export function pathologicalScale(): number { - return Number(process.env.PATHOLOGICAL_SCALE ?? "1"); + return readIntegerEnv("PATHOLOGICAL_SCALE", 1, { min: 1 }); } export function pathologicalCases(scale = 1): PathologicalCase[] { diff --git a/src/utils/markdownPipeline/parser/fastPaths/inlineFastPaths.test.ts b/src/utils/markdownPipeline/parser/fastPaths/inlineFastPaths.test.ts index 3f633de84..12be4316a 100644 --- a/src/utils/markdownPipeline/parser/fastPaths/inlineFastPaths.test.ts +++ b/src/utils/markdownPipeline/parser/fastPaths/inlineFastPaths.test.ts @@ -30,6 +30,7 @@ import remarkMath from "remark-math"; import remarkFrontmatter from "remark-frontmatter"; import { remarkInlineFastPaths } from "./remarkInlineFastPaths"; import { CORPORA, loadExamples } from "../../__tests__/spec/corpusRegistry"; +import { readIntegerEnv } from "@/test/envInteger"; const stock = unified() .use(remarkParse) @@ -149,7 +150,7 @@ describe("inline fast paths leave every parse unchanged (#1407)", () => { } }); - const SEED = Number(process.env.FAST_PATH_SEED ?? "1407"); + const SEED = readIntegerEnv("FAST_PATH_SEED", 1407); const TOKENS = [ "[", "]", "![", "(", ")", "<", ">", "^", " ", "\t", "\n", "\n\n", "a", "b", "\\", "`", "``", "*", "**", "_", "__", ":", "/", "中", "😀", "[^1]", "[^1]: n\n\n", "[a]: /u\n\n", "> ", "- ", From 51f9e730a6c18333a7e3861b8c8be436d0f53da8 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 13:48:19 +0800 Subject: [PATCH 06/11] fix(editor): refuse any document change appended to an undo or redo The previous commit kept Tiptap's clearDocument plugin off history transactions by setting its opt-out meta. The branch's cross-model audit found a second plugin with the same mechanism: footnote cleanup deletes a definition whose last reference an edit removed, and an undo that removes a reference is such an edit. Editing an unreferenced definition, adding its reference, then undoing twice deleted the definition on the first undo and threw `RangeError: Position 18 out of range` on the second. Two instances make it a class, and the class belongs to the combination, not to either plugin: prosemirror-history files a transaction appended to an undo in the redo branch without remapping the undo branch, so ANY appendTransaction that changes the document in response to an undo corrupts what is left. undoIntegrity now records the history transaction being applied, in the extension dispatch hook every view dispatch passes through, and a filterTransaction refuses any other document-changing transaction appended while it applies. A transaction dispatched during that window comes through the hook as its own root and is not refused; stored marks and metadata carry no steps and pass. Tests pin both regressions and that the normalizations still run on the ordinary edits they exist for. --- src/plugins/undoIntegrity/tiptap.test.ts | 84 +++++++++++++++++++----- src/plugins/undoIntegrity/tiptap.ts | 83 ++++++++++++++++------- 2 files changed, 129 insertions(+), 38 deletions(-) diff --git a/src/plugins/undoIntegrity/tiptap.test.ts b/src/plugins/undoIntegrity/tiptap.test.ts index 73859eb5f..483031959 100644 --- a/src/plugins/undoIntegrity/tiptap.test.ts +++ b/src/plugins/undoIntegrity/tiptap.test.ts @@ -1,21 +1,22 @@ /** * Undo restores the document it recorded, and the undo history stays usable. * - * Tiptap's core `clearDocument` plugin turns a document emptied by - * "select everything, then delete" back into a plain paragraph. It keys on the - * OLD selection covering the whole document and the NEW document being empty, - * so an UNDO taken with the text fully selected qualifies too, whenever it - * leaves an empty list item or heading behind. The plugin then lifted that - * item in an appended transaction. + * prosemirror-history files a transaction APPENDED to an undo in the redo + * branch without remapping what remains in the undo branch, so a plugin that + * rewrites the document in response to an undo leaves the next undo replaying + * steps recorded for a document that no longer exists. Two did: * - * prosemirror-history files a transaction appended to an undo in the REDO - * branch without remapping what remains in the undo branch. The next undo - * applied steps recorded for a document that no longer existed and threw - * `RangeError: Position 6 out of range` — found by the soak's editing fuzz - * (#1407, seed 4). Before it threw, the first undo had already produced a - * document that never existed: a paragraph where the empty list item was. + * - Tiptap core's `clearDocument` turns an emptied, fully selected document + * back into a paragraph, and lifted the empty list item an undo restored. + * The next undo threw `RangeError: Position 6 out of range` — found by the + * soak's editing fuzz (#1407, seed 4). + * - Footnote cleanup deletes a definition whose last reference was removed, + * and deleted it when an undo removed the reference: `Position 18`. + * + * In both, the first undo had already shown a document that never existed. */ import { afterEach, describe, expect, it } from "vitest"; +import { closeHistory } from "@tiptap/pm/history"; import { Selection } from "@tiptap/pm/state"; import { createTypingSession, type TypingSession } from "@/test/typingHarness"; @@ -88,9 +89,62 @@ describe("undo with the whole document selected", () => { }); }); -// The guard is scoped to history transactions: the case clearDocument exists -// for must keep working. -describe("select everything, then delete", () => { +// The same mechanism through a second plugin: footnote cleanup deletes a +// definition whose last reference an edit removed, and an UNDO that removes a +// reference is such an edit. Found by the branch's cross-model audit, which is +// why the guard refuses every document change appended to a history +// transaction instead of opting one plugin out. +describe("undo that removes a footnote reference", () => { + function editDefinitionThenAddReference(): TypingSession { + const s = createTypingSession({ markdown: "Text here.\n\n[^1]: note\n" }); + // Loading is itself a history event; close it so the edit below is not + // grouped with the load, as it would not be for a user typing later. + s.editor.view.dispatch(closeHistory(s.editor.state.tr)); + let definitionTextEnd = -1; + s.editor.state.doc.descendants((node, pos) => { + if (node.type.name === "footnote_definition") definitionTextEnd = pos + node.nodeSize - 2; + }); + s.setCursor(definitionTextEnd); + s.type("x"); + const reference = s.editor.schema.nodes.footnote_reference.create({ label: "1" }); + // Away from the definition, so history records it as a separate event. + s.editor.view.dispatch(s.editor.state.tr.insert(5, reference)); + return s; + } + + const hasDefinition = (s: TypingSession): boolean => blockTypes(s).includes("footnote_definition"); + + it("keeps the definition the document had before the reference", () => { + session = editDefinitionThenAddReference(); + session.undo(); + expect(hasDefinition(session)).toBe(true); + expect(session.editor.state.doc.textContent).toBe("Text here.notex"); + }); + + it("undoes the definition edit too, without throwing", () => { + session = editDefinitionThenAddReference(); + session.undo(); + expect(() => session?.undo()).not.toThrow(); + expect(session.editor.state.doc.textContent).toBe("Text here.note"); + expect(hasDefinition(session)).toBe(true); + // The rest of the history (the harness's own load) still applies. + expect(() => undoAll(session as TypingSession)).not.toThrow(); + }); +}); + +// The guard is scoped to history transactions: the normalizations it keeps +// off an undo must still run on the user edits they exist for. +describe("normalizers on ordinary edits", () => { + it("still deletes a definition when the user deletes its only reference", () => { + session = createTypingSession({ markdown: "Text[^1] here.\n\n[^1]: note\n" }); + let referenceAt = -1; + session.editor.state.doc.descendants((node, pos) => { + if (node.type.name === "footnote_reference") referenceAt = pos; + }); + session.editor.view.dispatch(session.editor.state.tr.delete(referenceAt, referenceAt + 1)); + expect(blockTypes(session)).not.toContain("footnote_definition"); + }); + it("still turns an emptied heading back into a paragraph", () => { session = createTypingSession({ markdown: "# Title\n" }); selectAllText(session); diff --git a/src/plugins/undoIntegrity/tiptap.ts b/src/plugins/undoIntegrity/tiptap.ts index f65fa9fc0..b1a61ee58 100644 --- a/src/plugins/undoIntegrity/tiptap.ts +++ b/src/plugins/undoIntegrity/tiptap.ts @@ -1,44 +1,81 @@ /** - * Undo integrity — keep document normalizers from rewriting an undo or redo. + * Undo integrity — no plugin may rewrite the document an undo or redo restores. * * Purpose: an undo must restore the document it recorded, and leave the rest * of the history applicable to the result. * - * Tiptap core's `clearDocument` plugin (the `keymap` core extension) turns a - * document emptied by select-all-and-delete back into a plain paragraph. Its - * test is "the old selection covered everything and the new document is - * empty", which an UNDO taken with the text fully selected also passes whenever - * it leaves an empty list item or heading behind — so it lifted that item in an - * appended transaction. prosemirror-history files a transaction appended to an - * undo in the redo branch WITHOUT remapping the undo branch, so the next undo - * replayed steps recorded for a document that no longer existed: - * `RangeError: Position 6 out of range` (#1407 soak, editing fuzz seed 4). + * prosemirror-history files a transaction APPENDED to an undo in the redo + * branch without remapping what remains in the undo branch. Any + * `appendTransaction` that changes the document in response to an undo + * therefore leaves the next undo replaying steps recorded for a document that + * no longer exists. Two plugins did it, each through a rule that is right for + * a user edit and wrong for an undo: + * - Tiptap core's `clearDocument` turns an emptied, fully selected document + * back into a paragraph — and lifted the empty list item an undo restored: + * `RangeError: Position 6 out of range` (#1407 soak, editing fuzz seed 4). + * - Footnote cleanup deletes a definition whose last reference an edit + * removed — and deleted it when an undo removed the reference: + * `RangeError: Position 18 out of range`. * * Key decisions: - * - Opt out with the plugin's own `preventClearDocument` meta rather than - * disabling the core extension, which also owns Backspace/Delete/Enter. - * - Tag in the extension `dispatchTransaction` hook, which every view - * dispatch passes through — the undo/redo commands and prosemirror-history's - * own `beforeinput` handler alike — before the state applies it. - * - Only history transactions are tagged; select-all-and-delete keeps its - * normalization. + * - Refuse the appended change centrally (`filterTransaction`) instead of + * opting plugins out one by one: the defect belongs to the combination of + * appendTransaction and history, so every present and future normalizer + * has it, including Tiptap's own. + * - The history transaction being applied is known because every view + * dispatch passes through the extension `dispatchTransaction` hook first — + * the undo/redo commands and prosemirror-history's own `beforeinput` + * handler alike — and applying it (including every appendTransaction) is + * synchronous inside `next`. A transaction dispatched while it applies + * (a view update reacting to it) comes through the hook again and is its + * own root, so it is not refused. + * - Only DOCUMENT changes are refused. Stored marks and plugin metadata + * appended to an undo carry no steps and do not touch history. + * - The recorded document was already normalized when it was recorded, so + * undo returns to a state the normalizers accepted; the next user edit + * runs them again. * * @coordinates-with services/assembly/tiptapExtensions.ts — registers it * @module plugins/undoIntegrity/tiptap */ import { Extension } from "@tiptap/core"; import { isHistoryTransaction } from "@tiptap/pm/history"; +import { Plugin, PluginKey, type Transaction } from "@tiptap/pm/state"; -/** The meta Tiptap's `clearDocument` plugin honours as an opt-out. */ -const PREVENT_CLEAR_DOCUMENT = "preventClearDocument"; +interface UndoIntegrityStorage { + /** The undo/redo transaction currently being applied, if any. */ + historyRoot: Transaction | null; +} -export const undoIntegrityExtension = Extension.create({ +const undoIntegrityKey = new PluginKey("undoIntegrity"); + +export const undoIntegrityExtension = Extension.create, UndoIntegrityStorage>({ name: "undoIntegrity", + addStorage() { + return { historyRoot: null }; + }, + dispatchTransaction({ transaction, next }) { - if (isHistoryTransaction(transaction)) { - transaction.setMeta(PREVENT_CLEAR_DOCUMENT, true); + const outer = this.storage.historyRoot; + this.storage.historyRoot = isHistoryTransaction(transaction) ? transaction : null; + try { + next(transaction); + } finally { + this.storage.historyRoot = outer; } - next(transaction); + }, + + addProseMirrorPlugins() { + const storage = this.storage; + return [ + new Plugin({ + key: undoIntegrityKey, + filterTransaction(transaction) { + const root = storage.historyRoot; + return root === null || transaction === root || !transaction.docChanged; + }, + }), + ]; }, }); From b12e5c3c39dab6f624eef447551f60887ea3a6ae Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 13:48:19 +0800 Subject: [PATCH 07/11] fix(markdown): write text line endings that would make a blank line as references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The serializer wrote a text line ending raw. One is a soft break and reads back as itself, but a line ending that starts a paragraph, ends it, or sits beside another one makes a blank line, which is structure: the parser drops it at a paragraph's edge and splits the paragraph at one inside. Inside a list item the empty first line also meant the item started with a blank line, so a nested list could not interrupt the paragraph above it. The branch's cross-model audit reproduced that from `- b\n 1. x`, against the new list join, which only looked for an empty paragraph. The root cause is the raw line ending, so a small text handler now writes exactly those line endings as character references and leaves a lone soft break, LF or CRLF, raw. It is a post-pass on upstream's escaping rather than an `unsafe` pattern, because upstream's hard-break handler reads any line-ending pattern in scope as "no line endings here" and degraded every hard break to a space: the spec corpus caught that attempt on 32 examples. CommonMark example 39 (`foo bar`) was ledgered as exactly this defect ("entity-newline injection … one paragraph reparses as two blocks"). It now round-trips, and its six ledger entries are removed, as the ledger requires. --- .../__tests__/spec/specRoundtripDeltas.json | 60 ------------- .../listInterruptJoin.test.ts | 10 +++ .../serializer.lineEndings.test.ts | 65 ++++++++++++++ src/utils/markdownPipeline/serializer.ts | 5 ++ src/utils/markdownPipeline/serializerText.ts | 85 +++++++++++++++++++ 5 files changed, 165 insertions(+), 60 deletions(-) create mode 100644 src/utils/markdownPipeline/serializer.lineEndings.test.ts create mode 100644 src/utils/markdownPipeline/serializerText.ts diff --git a/src/utils/markdownPipeline/__tests__/spec/specRoundtripDeltas.json b/src/utils/markdownPipeline/__tests__/spec/specRoundtripDeltas.json index 20110b016..920a60383 100644 --- a/src/utils/markdownPipeline/__tests__/spec/specRoundtripDeltas.json +++ b/src/utils/markdownPipeline/__tests__/spec/specRoundtripDeltas.json @@ -320,36 +320,6 @@ "verdict": "model-limit", "reason": "Inline mark model: nested same-type emphasis, mark order around links, or empty-text inline constructs cannot be represented distinctly by ProseMirror marks, so the re-parse reads a flattened/re-ordered inline shape." }, - { - "exampleId": "cm-39", - "path": "root", - "kind": "child-count", - "detail": "1 vs 2", - "inputValue": 1, - "outputValue": 2, - "verdict": "defect", - "reason": "Entity-newline injection: numeric entities for newlines ( ) serialize as REAL newlines, so one paragraph reparses as two blocks — fixable corruption of the author's structure; ratcheted." - }, - { - "exampleId": "cm-39", - "path": "root.children[0].children[0]", - "kind": "attribute", - "detail": "value", - "inputValue": "foo\n\nbar", - "outputValue": "foo", - "verdict": "defect", - "reason": "Entity-newline injection: numeric entities for newlines ( ) serialize as REAL newlines, so one paragraph reparses as two blocks — fixable corruption of the author's structure; ratcheted." - }, - { - "exampleId": "cm-39", - "path": "root.children[1]", - "kind": "missing", - "detail": "absent in document", - "inputValue": "__undefined__", - "outputValue": "paragraph", - "verdict": "defect", - "reason": "Entity-newline injection: numeric entities for newlines ( ) serialize as REAL newlines, so one paragraph reparses as two blocks — fixable corruption of the author's structure; ratcheted." - }, { "exampleId": "cm-407", "path": "root.children[0].children[0]", @@ -2930,36 +2900,6 @@ } ], "fidelity": [ - { - "exampleId": "cm-39", - "path": "root", - "kind": "child-count", - "detail": "1 vs 2", - "inputValue": 1, - "outputValue": 2, - "verdict": "defect", - "reason": "Entity-newline injection: numeric entities for newlines ( ) serialize as REAL newlines, so one paragraph reparses as two blocks — fixable corruption of the author's structure; ratcheted." - }, - { - "exampleId": "cm-39", - "path": "root.children[0].children[0]", - "kind": "attribute", - "detail": "value", - "inputValue": "foo\n\nbar", - "outputValue": "foo", - "verdict": "defect", - "reason": "Entity-newline injection: numeric entities for newlines ( ) serialize as REAL newlines, so one paragraph reparses as two blocks — fixable corruption of the author's structure; ratcheted." - }, - { - "exampleId": "cm-39", - "path": "root.children[1]", - "kind": "missing", - "detail": "absent in document", - "inputValue": "__undefined__", - "outputValue": "paragraph", - "verdict": "defect", - "reason": "Entity-newline injection: numeric entities for newlines ( ) serialize as REAL newlines, so one paragraph reparses as two blocks — fixable corruption of the author's structure; ratcheted." - }, { "exampleId": "cm-109", "path": "root.children[0].children[0]", diff --git a/src/utils/markdownPipeline/listInterruptJoin.test.ts b/src/utils/markdownPipeline/listInterruptJoin.test.ts index e64d4ee1f..c9fa03f65 100644 --- a/src/utils/markdownPipeline/listInterruptJoin.test.ts +++ b/src/utils/markdownPipeline/listInterruptJoin.test.ts @@ -74,6 +74,16 @@ describe("nested list after a paragraph in a list item", () => { expect(markdown).toBe(`- b\n\n ${start}. c\n`); }); + // An item whose text begins with a line ending also starts with a blank + // line: the serializer writes the character raw, so the marker line is + // empty. Found by the branch's cross-model audit. + it.each([" x", " x", " "])("keeps a nested item whose text starts with %s", (content) => { + const source = `- b\n 1. ${content}\n`; + const doc = parseMarkdown(schema, source); + const { back, want } = roundTrip(doc); + expect(back).toEqual(want); + }); + // The rule is narrow on purpose: a list that CAN interrupt keeps the tight // spelling authors wrote, so no existing document gains blank lines. it("leaves a list that can interrupt the paragraph tight", () => { diff --git a/src/utils/markdownPipeline/serializer.lineEndings.test.ts b/src/utils/markdownPipeline/serializer.lineEndings.test.ts new file mode 100644 index 000000000..76fd6a3b1 --- /dev/null +++ b/src/utils/markdownPipeline/serializer.lineEndings.test.ts @@ -0,0 +1,65 @@ +// @vitest-environment node +/** + * A line ending in paragraph text that would start or end a blank line is + * written as a character reference. + * + * The serializer wrote text line endings raw. A paragraph whose text begins or + * ends with one therefore gained an empty line the parser drops, and two in a + * row became a blank line that split the paragraph in two. Inside a list item + * the empty first line also meant the item "started with a blank line", so a + * nested list could no longer interrupt the paragraph above it and was read + * back as that paragraph's text (the branch's cross-model audit, reproducing it + * from `- b\n 1. x`). Such text only arrives through character + * references, which is exactly why it must leave through them too. + */ +import { describe, expect, it } from "vitest"; +import type { Root } from "mdast"; +import { getProductionSchema } from "@/test/productionSchema"; +import { parseMarkdown, serializeMarkdown } from "./adapter"; +import { serializeMdastToMarkdown } from "./serializer"; + +const schema = getProductionSchema(); + +const paragraphOf = (value: string): Root => ({ + type: "root", + children: [{ type: "paragraph", children: [{ type: "text", value }] }], +}); + +/** Text content of the first paragraph after serializing and reparsing. */ +function reparsedText(value: string): string { + const markdown = serializeMdastToMarkdown(paragraphOf(value)); + return parseMarkdown(schema, markdown).textContent; +} + +describe("line endings at the edges of paragraph text", () => { + it.each([ + ["a leading LF", "\nx"], + ["a leading CR", "\rx"], + ["a trailing LF", "x\n"], + ["a blank line inside", "a\n\nb"], + ["a blank line with spaces", "a\n \nb"], + ["only an LF", "\n"], + ])("keeps %s", (_label, value) => { + expect(reparsedText(value)).toBe(value); + }); + + it("keeps the paragraph whole when its text holds a blank line", () => { + const markdown = serializeMdastToMarkdown(paragraphOf("a\n\nb")); + expect(parseMarkdown(schema, markdown).childCount).toBe(1); + }); + + // A single soft line break is ordinary markdown and stays raw. + it.each(["a\nb", "a\r\nb"])("writes the single interior line break in %j raw", (value) => { + expect(serializeMdastToMarkdown(paragraphOf(value))).toBe(`${value}\n`); + }); + + it("keeps a CRLF blank line", () => { + expect(reparsedText("a\r\n\r\nb")).toBe("a\r\n\r\nb"); + }); + + it("round-trips the audit's list source", () => { + const doc = parseMarkdown(schema, "- b\n 1. x\n"); + const again = parseMarkdown(schema, serializeMarkdown(schema, doc)); + expect(again.toJSON()).toEqual(doc.toJSON()); + }); +}); diff --git a/src/utils/markdownPipeline/serializer.ts b/src/utils/markdownPipeline/serializer.ts index 1358888dc..1f05b111c 100644 --- a/src/utils/markdownPipeline/serializer.ts +++ b/src/utils/markdownPipeline/serializer.ts @@ -28,12 +28,14 @@ * @coordinates-with serializerHandlers.ts — custom image/link to-markdown handlers * @coordinates-with serializerAttention.ts — emphasis/strong/delete handlers * @coordinates-with listInterruptJoin.ts — blank line before a non-interrupting list + * @coordinates-with serializerText.ts — text line endings that would make a blank line * @module utils/markdownPipeline/serializer */ import { unified } from "unified"; import remarkStringify from "remark-stringify"; import { handleDelete, handleEmphasis, handleStrong } from "./serializerAttention"; +import { handleText } from "./serializerText"; import remarkGfm from "remark-gfm"; import remarkMath from "remark-math"; import remarkFrontmatter from "remark-frontmatter"; @@ -83,6 +85,9 @@ function buildSerializer() { emphasis: handleEmphasis, strong: handleStrong, delete: handleDelete, + // A text line ending that would make a blank line is written as a + // character reference (serializerText.ts). + text: handleText, ...tocToMarkdown.handlers, } as Record, // Joins are consulted last-first: listInterruptJoin can raise a captured diff --git a/src/utils/markdownPipeline/serializerText.ts b/src/utils/markdownPipeline/serializerText.ts new file mode 100644 index 000000000..423cf3e6c --- /dev/null +++ b/src/utils/markdownPipeline/serializerText.ts @@ -0,0 +1,85 @@ +/** + * Text serialization — line endings that would make a blank line. + * + * Purpose: write a `text` node so that the line endings it carries come back + * as text, never as block structure. + * + * Upstream writes a text line ending raw. One raw line ending is a soft break + * and reads back as itself. But a line ending that starts the paragraph, ends + * it, or sits directly beside another one makes a BLANK line, and a blank line + * is structure: the parser drops it at a paragraph's edge and splits the + * paragraph at one inside. In a list item the empty first line also means the + * item "starts with a blank line", so a nested list below it can no longer + * interrupt the paragraph and is read back as its text — found by the branch's + * cross-model audit from `- b\n 1. x`. Such text only arrives through + * character references, so it leaves through them. + * + * Key decisions: + * - Encode only the line endings that would make a blank line; a lone soft + * break, LF or CRLF, stays raw so ordinary documents do not change. + * - A post-pass on `state.safe`'s output, not an `unsafe` pattern: upstream's + * hard-break handler reads ANY `\n` pattern in scope as "line endings are + * not allowed here" and degrades every hard break to a space. + * - Spaces between two line endings need nothing here: upstream already + * encodes a space beside a line ending, and ` ` is not blank. + * + * @coordinates-with serializer.ts — installs this handler + * @coordinates-with listInterruptJoin.ts — the other half of "a list item's + * first line must not be blank" + * @module utils/markdownPipeline/serializerText + */ + +/** The slice of mdast-util-to-markdown's `State` this handler uses. */ +interface TextState { + safe: (value: string, info: TextInfo) => string; +} + +/** The characters around the node being serialized. */ +interface TextInfo { + before: string; + after: string; +} + +const LINE_ENDING = /\r\n|\r|\n/g; +const endsWithLineEnding = (value: string): boolean => /[\r\n]$/.test(value); +const startsWithLineEnding = (value: string): boolean => /^[\r\n]/.test(value); +const reference = (ending: string): string => + [...ending].map((char) => `&#x${char.charCodeAt(0).toString(16).toUpperCase()};`).join(""); + +/** + * `value` with every line ending that would make a blank line written as a + * character reference: one directly after a line ending (or after `before` + * ending in one), and one directly before a line ending (or before `after` + * starting with one). + */ +function encodeBlankLineEndings(value: string, before: string, after: string): string { + const endings = [...value.matchAll(LINE_ENDING)]; + if (endings.length === 0) return value; + + let out = ""; + let cursor = 0; + endings.forEach((match, index) => { + const start = match.index; + const end = start + match[0].length; + const previous = endings[index - 1]; + const next = endings[index + 1]; + const afterLineEnding = + previous !== undefined ? previous.index + previous[0].length === start : start === 0 && endsWithLineEnding(before); + const beforeLineEnding = + next !== undefined ? next.index === end : end === value.length && startsWithLineEnding(after); + + out += value.slice(cursor, start) + (afterLineEnding || beforeLineEnding ? reference(match[0]) : match[0]); + cursor = end; + }); + return out + value.slice(cursor); +} + +/** `text` handler: upstream's escaping, then blank-line line endings encoded. */ +export function handleText( + node: { value: string }, + _parent: unknown, + state: TextState, + info: TextInfo, +): string { + return encodeBlankLineEndings(state.safe(node.value, info), info.before, info.after); +} From 687216ab15260e7292986ed8685e54e1676ec899 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 13:48:19 +0800 Subject: [PATCH 08/11] test(gates): close the gaps the audit found in both empty-input detectors The branch's cross-model audit reproduced misses in the two gates added for the soak seed defect. check-workflow-input-fallbacks.test.mjs matched `inputs.x` with one regex over the raw expression, so `inputs['fuzz_seed']` went unseen, `|| ''` counted as a fallback, and a string literal that mentioned `inputs.x` was a finding. String literals are now masked before matching, bracket keys are read, every input in an expression needs a non-empty fallback of its own, and a fallback that is another input is checked in its turn. check-numeric-env-reads.test.mjs skipped any file not containing the exact text `process.env`, and did not see through `process["env"]`, `as string`, `!` or ``. It now prefilters on the word `process`, unwraps parentheses and type assertions, resolves string-keyed element access, and parses each file in its own dialect. Each gap has a self-test that failed before the change, and both gates were mutation-checked again: reverting the soak fallback, or the fuzz test's readIntegerEnv call, still turns them red. --- scripts/check-numeric-env-reads.test.mjs | 56 ++++++++++++---- .../check-workflow-input-fallbacks.test.mjs | 64 ++++++++++++++++--- 2 files changed, 101 insertions(+), 19 deletions(-) diff --git a/scripts/check-numeric-env-reads.test.mjs b/scripts/check-numeric-env-reads.test.mjs index 5bf05d11d..c8fe90e19 100644 --- a/scripts/check-numeric-env-reads.test.mjs +++ b/scripts/check-numeric-env-reads.test.mjs @@ -11,7 +11,8 @@ * * It walks a TypeScript AST rather than grepping, so an explanation of the * defect in a comment or a string — like the one above — is prose, not a - * finding. Only files that mention `process.env` are parsed. Tracked and + * finding. It sees through parentheses, type assertions and `process["env"]`. + * Only files that mention `process` are parsed. Tracked and * untracked-not-ignored files are listed by `git ls-files`, so generated and * ignored trees cannot contribute. * @@ -30,20 +31,39 @@ import ts from "typescript"; const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const CONVERTERS = new Set(["Number", "parseInt", "parseFloat", "Number.parseInt", "Number.parseFloat"]); -/** `a.b.c` for a chain of identifiers, or null for anything else. */ +/** `node` without the wrappers that do not change its value: parentheses and type assertions. */ +function unwrap(node) { + let current = node; + while ( + ts.isParenthesizedExpression(current) || + ts.isAsExpression(current) || + ts.isNonNullExpression(current) || + ts.isSatisfiesExpression(current) || + ts.isTypeAssertionExpression(current) + ) { + current = current.expression; + } + return current; +} + +/** `a.b.c` for a chain of identifiers and string-keyed element accesses, or null. */ function dottedName(node) { - if (ts.isIdentifier(node)) return node.text; - if (ts.isPropertyAccessExpression(node)) { - const left = dottedName(node.expression); - return left === null ? null : `${left}.${node.name.text}`; + const current = unwrap(node); + if (ts.isIdentifier(current)) return current.text; + if (ts.isPropertyAccessExpression(current)) { + const left = dottedName(current.expression); + return left === null ? null : `${left}.${current.name.text}`; + } + if (ts.isElementAccessExpression(current) && ts.isStringLiteralLike(current.argumentExpression)) { + const left = dottedName(current.expression); + return left === null ? null : `${left}.${current.argumentExpression.text}`; } return null; } -/** Whether `node` reads `process.env.X` / `process.env["X"]`, possibly with a `??`/`||` default. */ +/** Whether `node` reads a variable off `process.env`, possibly with a `??`/`||` default. */ function readsProcessEnv(node) { - let current = node; - while (ts.isParenthesizedExpression(current)) current = current.expression; + const current = unwrap(node); if ( ts.isBinaryExpression(current) && (current.operatorToken.kind === ts.SyntaxKind.QuestionQuestionToken || @@ -58,10 +78,18 @@ function readsProcessEnv(node) { return false; } +/** The parser dialect for a file: `x` is a type assertion in `.ts` and JSX in `.tsx`. */ +function scriptKind(file) { + if (/\.[jt]sx$/.test(file)) return ts.ScriptKind.TSX; + if (/\.[mc]?ts$/.test(file)) return ts.ScriptKind.TS; + return ts.ScriptKind.JS; +} + /** `file:line` for every numeric conversion applied directly to an environment read. */ function numericEnvReads(source, file) { - if (!source.includes("process.env")) return []; - const sourceFile = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + // A cheap filter that cannot hide a read: every form reaches the global `process`. + if (!/\bprocess\b/.test(source)) return []; + const sourceFile = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, scriptKind(file)); const out = []; const visit = (node) => { const converted = @@ -117,6 +145,12 @@ describe("SELF-TEST: the detector", () => { 'const k = Number(process.env["KNOB"] || "3");', "const p = +process.env.PORT;", "const g = Number(globalThis.process.env.G);", + // Found by the branch's cross-model audit. + "const a = Number(process . env.X);", + 'const b = Number(process["env"].X);', + "const c = Number(process.env.X as string);", + "const d = Number(process.env.X!);", + "const e = parseInt(process.env.X, 10);", ])("flags %s", (line) => { expect(numericEnvReads(line, "x.ts")).toEqual(["x.ts:1"]); }); diff --git a/scripts/check-workflow-input-fallbacks.test.mjs b/scripts/check-workflow-input-fallbacks.test.mjs index 9c5e5ef1a..6d6049283 100644 --- a/scripts/check-workflow-input-fallbacks.test.mjs +++ b/scripts/check-workflow-input-fallbacks.test.mjs @@ -41,20 +41,48 @@ import { isMap, isPair, isScalar, parseDocument, visit } from "yaml"; const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const DIR = path.join(REPO, ".github/workflows"); -/** An `inputs.` read inside one `${{ … }}` expression. */ -const INPUT_READ = /\b(?:github\.event\.)?inputs\.[A-Za-z_][A-Za-z0-9_-]*/; const EXPRESSION = /\$\{\{([\s\S]*?)\}\}/g; const MARKER = /^\s*input-empty-ok:\s*(\S.*)?$/; +/** A single-quoted expression string; `''` is an escaped quote. */ +const STRING_LITERAL = /'(?:[^']|'')*'/g; +/** An input read, after string literals are masked: `inputs.x` or `inputs['x']`. */ +const INPUT_READ = /\b(?:github\.event\.)?inputs\s*(?:\.\s*[A-Za-z_][A-Za-z0-9_-]*|\[\s*@str\d+@\s*\])/g; +/** The first `||` operand after a read: a masked literal, or any other token. */ +const FALLBACK = /\|\|\s*(?:@str(\d+)@|[^\s|)]+)/; + +/** + * An expression body with each string literal replaced by `@str@`, so a + * literal that merely mentions `inputs.x` is not a read, and a bracket key + * (`inputs['x']`) or a fallback's emptiness can still be checked. + */ +function maskStrings(body) { + const literals = []; + const masked = body.replace(STRING_LITERAL, (literal) => { + literals.push(literal.slice(1, -1).replace(/''/g, "'")); + return `@str${literals.length - 1}@`; + }); + return { masked, literals }; +} /** Every `${{ … }}` body in `text` that reads an input. */ function inputExpressions(text) { - return [...text.matchAll(EXPRESSION)].map((m) => m[1]).filter((body) => INPUT_READ.test(body)); + return [...text.matchAll(EXPRESSION)] + .map((match) => match[1]) + .filter((body) => maskStrings(body).masked.match(INPUT_READ) !== null); } -/** Whether an expression supplies a fallback after the input it reads. */ -function hasFallback(body) { - const read = INPUT_READ.exec(body); - return read !== null && /\|\|\s*\S/.test(body.slice(read.index + read[0].length)); +/** + * Whether every input read in `body` is followed by a fallback that is not the + * empty string. A fallback that is itself an input read is checked in its own + * turn, so `inputs.a || inputs.b` still needs `inputs.b` to fall back. + */ +function everyReadHasFallback(body) { + const { masked, literals } = maskStrings(body); + return [...masked.matchAll(INPUT_READ)].every((read) => { + const fallback = FALLBACK.exec(masked.slice(read.index + read[0].length)); + if (fallback === null) return false; + return fallback[1] === undefined || literals[Number(fallback[1])] !== ""; + }); } /** @@ -74,7 +102,7 @@ function findings(source, file) { if (!isPair(entry) || !isScalar(entry.value) || typeof entry.value.value !== "string") continue; const name = String(isScalar(entry.key) ? entry.key.value : "?"); for (const body of inputExpressions(entry.value.value)) { - if (hasFallback(body)) continue; + if (everyReadHasFallback(body)) continue; const marker = MARKER.exec(entry.value.comment ?? ""); if (marker && marker[1]) continue; out.push({ @@ -150,6 +178,26 @@ describe("SELF-TEST: the checker", () => { expect(findings(wf(" A: plain", run), "t.yml")).toHaveLength(1); }); + // Found by the branch's cross-model audit. + it("flags bracket access to an input", () => { + expect(findings(wf(" S: ${{ inputs['fuzz_seed'] }}"), "t.yml")).toHaveLength(1); + expect(findings(wf(" S: ${{ github.event.inputs[ 'fuzz_seed' ] }}"), "t.yml")).toHaveLength(1); + expect(findings(wf(" S: ${{ inputs['fuzz_seed'] || '1' }}"), "t.yml")).toEqual([]); + }); + + it("does not count an empty fallback", () => { + expect(findings(wf(" S: ${{ inputs.fuzz_seed || '' }}"), "t.yml")).toHaveLength(1); + }); + + it("checks a fallback that is itself an input", () => { + expect(findings(wf(" S: ${{ inputs.a || inputs.b }}"), "t.yml")).toHaveLength(1); + expect(findings(wf(" S: ${{ inputs.a || inputs.b || 'c' }}"), "t.yml")).toEqual([]); + }); + + it("ignores `inputs.x` inside a string literal", () => { + expect(findings(wf(" S: ${{ format('see inputs.x') }}"), "t.yml")).toEqual([]); + }); + it("ignores env values that read no input", () => { expect(findings(wf(" GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}"), "t.yml")).toEqual([]); }); From af8476184e81bcb3865e42305ae79bd0678f8d00 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 14:10:53 +0800 Subject: [PATCH 09/11] fix(editor): make VMark's normalizers stand down on undo however it is applied Round 2 of the branch's cross-model audit: the undoIntegrity guard lives in the extension dispatch hook, so an undo applied with `state.apply` and never dispatched bypasses it. Applying the audit's footnote setup that way still deleted the definition on the first undo and threw on the second. ProseMirror's filterTransaction cannot tell a transaction appended in this batch from the next root transaction, so a central guard cannot cover that path; no VMark code applies transactions that way. What CAN hold on every path is the normalizers VMark owns declining to react to a history batch at all. plugins/shared/historyBatch.ts detects one, including a transaction appended to an undo, which a plugin receives in a later round without the undo itself. Footnote cleanup and blankLinesGuard, the two VMark appendTransaction plugins that change the document outside IME composition, now check it. blankLinesGuard had the same shape: an undo that restored a block reset its captured blank-line count. Tests apply the undo directly and fail without the checks. The footnote plugin's change fits inside its size baseline, which ratchets down 384 -> 381. undoIntegrity's header states the boundary, and two assembly comments that still described the clearDocument-only guard are corrected. --- scripts/file-size-baseline.json | 2 +- .../blankLinesGuard/blankLinesGuard.test.ts | 22 ++++++ .../blankLinesGuard/blankLinesGuard.ts | 4 + src/plugins/footnotePopup/tiptap.ts | 17 ++--- src/plugins/shared/historyBatch.test.ts | 73 +++++++++++++++++++ src/plugins/shared/historyBatch.ts | 37 ++++++++++ src/plugins/undoIntegrity/tiptap.test.ts | 18 ++++- src/plugins/undoIntegrity/tiptap.ts | 8 ++ .../assembly/tiptapExtensions.test.ts | 2 +- src/services/assembly/tiptapExtensions.ts | 4 +- 10 files changed, 172 insertions(+), 15 deletions(-) create mode 100644 src/plugins/shared/historyBatch.test.ts create mode 100644 src/plugins/shared/historyBatch.ts diff --git a/scripts/file-size-baseline.json b/scripts/file-size-baseline.json index 1a137bf4d..082733a1b 100644 --- a/scripts/file-size-baseline.json +++ b/scripts/file-size-baseline.json @@ -53,7 +53,7 @@ "src/plugins/codemirror/tableTabNav.ts": 385, "src/plugins/compositionGuard/tiptap.ts": 403, "src/plugins/editorPlugins.tiptap.ts": 320, - "src/plugins/footnotePopup/tiptap.ts": 384, + "src/plugins/footnotePopup/tiptap.ts": 381, "src/plugins/imagePasteToast/ImagePasteToastView.ts": 305, "src/plugins/latex/MathInlineNodeView.ts": 565, "src/plugins/mediaPopup/MediaPopupView.ts": 480, diff --git a/src/plugins/blankLinesGuard/blankLinesGuard.test.ts b/src/plugins/blankLinesGuard/blankLinesGuard.test.ts index 89a6cf67d..77007cefb 100644 --- a/src/plugins/blankLinesGuard/blankLinesGuard.test.ts +++ b/src/plugins/blankLinesGuard/blankLinesGuard.test.ts @@ -7,6 +7,7 @@ // leaving parse-captured values and in-place content edits untouched. import { describe, it, expect, beforeEach } from "vitest"; import { EditorState, TextSelection } from "@tiptap/pm/state"; +import { history, undo } from "@tiptap/pm/history"; import { Schema, type Node as PMNode } from "@tiptap/pm/model"; import { blankLinesGuard } from "./blankLinesGuard"; @@ -118,3 +119,24 @@ describe("blankLinesGuard", () => { expect(blanksOf(next)).toEqual([null, 4]); }); }); + +// An undo restores the attributes it recorded. Nulling them in response would +// also append a change to the undo, which prosemirror-history files without +// remapping the rest of the undo branch (plugins/undoIntegrity) — so the guard +// stands down on a history batch however the transaction is applied. +describe("blankLinesGuard on undo", () => { + it("leaves the blank-line count an undo restores", () => { + let state = EditorState.create({ + schema, + doc: docWith({ text: "Hello", blank: 3 }), + plugins: [history(), blankLinesGuard()], + }); + // Delete the paragraph's content and structure, then undo it back. + state = state.apply(state.tr.replaceWith(0, state.doc.content.size, schema.node("paragraph"))); + expect(blanksOf(state)).toEqual([null]); + undo(state, (tr) => { + state = state.apply(tr); + }); + expect(blanksOf(state)).toEqual([3]); + }); +}); diff --git a/src/plugins/blankLinesGuard/blankLinesGuard.ts b/src/plugins/blankLinesGuard/blankLinesGuard.ts index 04e323972..a7bf55782 100644 --- a/src/plugins/blankLinesGuard/blankLinesGuard.ts +++ b/src/plugins/blankLinesGuard/blankLinesGuard.ts @@ -30,6 +30,7 @@ import { Plugin, PluginKey } from "@tiptap/pm/state"; import type { Transaction } from "@tiptap/pm/state"; import type { Node as PMNode } from "@tiptap/pm/model"; +import { isHistoryBatch } from "@/plugins/shared/historyBatch"; const blankLinesGuardKey = new PluginKey("blankLinesGuard"); @@ -78,6 +79,9 @@ export function blankLinesGuard(): Plugin { // setContentWithoutHistory marks these `preventUpdate`; genuine user // edits (split, paste) never set it. if (transactions.some((t) => t.getMeta("preventUpdate"))) return null; + // An undo/redo restores the attributes it recorded; resetting them here + // would also corrupt the undo history (plugins/shared/historyBatch). + if (isHistoryBatch(transactions)) return null; const ranges = changedRanges(transactions); if (ranges.length === 0) return null; diff --git a/src/plugins/footnotePopup/tiptap.ts b/src/plugins/footnotePopup/tiptap.ts index 9ebd64767..d68246f7b 100644 --- a/src/plugins/footnotePopup/tiptap.ts +++ b/src/plugins/footnotePopup/tiptap.ts @@ -13,7 +13,8 @@ * do not race on shared module timers. * - Popup uses FootnotePopupView (DOM-based, not React) for performance * - appendTransaction handles footnote deletion + renumbering in a single atomic step - * - appendTransaction skips during IME composition to avoid disrupting CJK input + * - appendTransaction skips IME composition (would disrupt CJK input) and undo/redo + * batches (would corrupt history; plugins/shared/historyBatch) * - Footnote references and definitions are bidirectionally linked for navigation * * @coordinates-with FootnotePopupView.ts — DOM construction and event handling for the popup @@ -30,17 +31,11 @@ import type { Node as PMNode, NodeType, Slice } from "@tiptap/pm/model"; import type { EditorView } from "@tiptap/pm/view"; import type { StoreApi } from "@/plugins/shared/types"; import type { FootnotePopupState } from "@/plugins/shared/popupPorts"; -import { - HOVER_OPEN_DELAY_MS, - HOVER_CLOSE_DELAY_MS, - getHoverState, - clearHoverTimeout, - clearCloseTimeout, - resetHoverState, -} from "./hoverState"; +import { HOVER_OPEN_DELAY_MS, HOVER_CLOSE_DELAY_MS, getHoverState, clearHoverTimeout, clearCloseTimeout, resetHoverState } from "./hoverState"; import { FootnotePopupView } from "./FootnotePopupView"; import { collectFootnoteNodes, createCleanupAndRenumberTransaction, createRenumberTransaction, hasRefCountDropped } from "./tiptapCleanup"; import { findFootnoteDefinition, findFootnoteReference, getFootnoteDefFromTarget, getFootnoteRefFromTarget, scrollToPosition } from "./tiptapDomUtils"; +import { isHistoryBatch } from "@/plugins/shared/historyBatch"; import "./footnote-popup.css"; export const footnotePopupPluginKey = new PluginKey("footnotePopup"); @@ -308,7 +303,9 @@ export const footnotePopupExtension = Extension.create({ if (!refType || !defType) return null; const docChanged = transactions.some((tr) => tr.docChanged); - if (!docChanged && !cleanupPending) return null; + // An undo/redo restores recorded footnotes; cleanup would rewrite it and + // corrupt the undo history. A pending cleanup waits for the next edit. + if ((!docChanged && !cleanupPending) || isHistoryBatch(transactions)) return null; // Skip during IME composition — dispatching transactions mid-composition // can cause ProseMirror to reconcile the DOM, disrupting active CJK input diff --git a/src/plugins/shared/historyBatch.test.ts b/src/plugins/shared/historyBatch.test.ts new file mode 100644 index 000000000..605cc0359 --- /dev/null +++ b/src/plugins/shared/historyBatch.test.ts @@ -0,0 +1,73 @@ +// @vitest-environment node +/** + * A normalizer's `appendTransaction` can tell when it is being asked to react + * to an undo or redo — directly, or to a transaction already appended to one. + */ +import { describe, expect, it } from "vitest"; +import { Schema } from "@tiptap/pm/model"; +import { EditorState, Plugin, type Transaction } from "@tiptap/pm/state"; +import { history, redo, undo } from "@tiptap/pm/history"; +import { isHistoryBatch } from "./historyBatch"; + +const schema = new Schema({ + nodes: { doc: { content: "paragraph+" }, paragraph: { content: "text*" }, text: {} }, +}); + +/** Record the batch every appendTransaction call receives, and optionally append once. */ +function recorder(appendOnFirstHistoryCall: boolean) { + const seen: boolean[][] = []; + let appended = false; + const plugin = new Plugin({ + appendTransaction(transactions: readonly Transaction[], _old, newState) { + seen.push([isHistoryBatch(transactions)]); + if (appendOnFirstHistoryCall && !appended && isHistoryBatch(transactions)) { + appended = true; + return newState.tr.setMeta("marker", true); + } + return null; + }, + }); + return { plugin, seen }; +} + +function typedState(plugins: Plugin[]): EditorState { + let state = EditorState.create({ schema, plugins: [history(), ...plugins] }); + state = state.apply(state.tr.insertText("abc", 1)); + return state; +} + +describe("isHistoryBatch", () => { + it("is false for an ordinary edit", () => { + const { plugin, seen } = recorder(false); + typedState([plugin]); + expect(seen).toEqual([[false]]); + }); + + it("is true for an undo and for a redo", () => { + const { plugin, seen } = recorder(false); + let state = typedState([plugin]); + undo(state, (tr) => { + state = state.apply(tr); + }); + redo(state, (tr) => { + state = state.apply(tr); + }); + expect(seen.slice(1)).toEqual([[true], [true]]); + }); + + // The second loop iteration hands a plugin only the transactions appended + // since its last call, without the undo itself. + it("is true for a transaction appended to an undo", () => { + const first = recorder(true); + const second = recorder(false); + let state = typedState([second.plugin, first.plugin]); + undo(state, (tr) => { + state = state.apply(tr); + }); + expect(second.seen.slice(1)).toEqual([[true], [true]]); + }); + + it("is false for an empty batch", () => { + expect(isHistoryBatch([])).toBe(false); + }); +}); diff --git a/src/plugins/shared/historyBatch.ts b/src/plugins/shared/historyBatch.ts new file mode 100644 index 000000000..b955d9c85 --- /dev/null +++ b/src/plugins/shared/historyBatch.ts @@ -0,0 +1,37 @@ +/** + * Purpose: let a document normalizer stand down when the batch it is reacting + * to is an undo or redo. + * + * An `appendTransaction` that changes the document in response to an undo + * corrupts the undo history: prosemirror-history files the appended change in + * the redo branch without remapping what remains in the undo branch, and the + * next undo replays steps against a document that no longer exists + * (`RangeError: Position N out of range`). It also shows a document the user + * never had, since the undo was supposed to restore a recorded one. + * + * `plugins/undoIntegrity` refuses such appends for every transaction dispatched + * through the editor. This is the half that holds however a transaction is + * applied — including `state.apply` with no view — for the normalizers VMark + * owns: each checks its batch and returns null. + * + * @coordinates-with plugins/undoIntegrity/tiptap.ts — the central, dispatch-level guard + * @coordinates-with plugins/footnotePopup/tiptap.ts — footnote cleanup + * @coordinates-with plugins/blankLinesGuard/blankLinesGuard.ts — blank-line reset + * @module plugins/shared/historyBatch + */ +import { isHistoryTransaction } from "@tiptap/pm/history"; +import type { Transaction } from "@tiptap/pm/state"; + +/** + * Whether `transactions` — an `appendTransaction` batch — contains an undo or + * redo, or a transaction appended to one. The second case matters: after the + * first round of appends, ProseMirror passes a plugin only the transactions + * added since its last call, which no longer include the undo itself. + */ +export function isHistoryBatch(transactions: readonly Transaction[]): boolean { + return transactions.some((tr) => { + if (isHistoryTransaction(tr)) return true; + const root: unknown = tr.getMeta("appendedTransaction"); + return root !== undefined && isHistoryTransaction(root as Transaction); + }); +} diff --git a/src/plugins/undoIntegrity/tiptap.test.ts b/src/plugins/undoIntegrity/tiptap.test.ts index 483031959..b3e97b75c 100644 --- a/src/plugins/undoIntegrity/tiptap.test.ts +++ b/src/plugins/undoIntegrity/tiptap.test.ts @@ -16,7 +16,7 @@ * In both, the first undo had already shown a document that never existed. */ import { afterEach, describe, expect, it } from "vitest"; -import { closeHistory } from "@tiptap/pm/history"; +import { closeHistory, undo } from "@tiptap/pm/history"; import { Selection } from "@tiptap/pm/state"; import { createTypingSession, type TypingSession } from "@/test/typingHarness"; @@ -130,6 +130,22 @@ describe("undo that removes a footnote reference", () => { // The rest of the history (the harness's own load) still applies. expect(() => undoAll(session as TypingSession)).not.toThrow(); }); + + // The dispatch guard cannot see a transaction applied without dispatching + // it; VMark's own normalizers stand down on a history batch by themselves + // (plugins/shared/historyBatch.ts). Round 2 of the branch's audit. + it("keeps the definition when the undo is applied without the editor's dispatch", () => { + session = editDefinitionThenAddReference(); + const { view } = session.editor; + const applyDirectly = () => + undo(view.state, (tr) => { + view.updateState(view.state.apply(tr)); + }); + applyDirectly(); + expect(hasDefinition(session)).toBe(true); + expect(applyDirectly).not.toThrow(); + expect(session.editor.state.doc.textContent).toBe("Text here.note"); + }); }); // The guard is scoped to history transactions: the normalizations it keeps diff --git a/src/plugins/undoIntegrity/tiptap.ts b/src/plugins/undoIntegrity/tiptap.ts index b1a61ee58..5b604a52a 100644 --- a/src/plugins/undoIntegrity/tiptap.ts +++ b/src/plugins/undoIntegrity/tiptap.ts @@ -35,6 +35,14 @@ * undo returns to a state the normalizers accepted; the next user edit * runs them again. * + * Boundary: the guard needs the dispatch context. ProseMirror's + * `filterTransaction` cannot tell a transaction appended in this batch from + * the next root transaction, so a transaction applied with `state.apply` and + * never dispatched is outside it — no VMark code does that. VMark's own + * normalizers also stand down on a history batch by themselves + * (plugins/shared/historyBatch), which holds on any path. + * + * @coordinates-with plugins/shared/historyBatch.ts — the path-independent half * @coordinates-with services/assembly/tiptapExtensions.ts — registers it * @module plugins/undoIntegrity/tiptap */ diff --git a/src/services/assembly/tiptapExtensions.test.ts b/src/services/assembly/tiptapExtensions.test.ts index 2440cd76d..bc4e8a7b0 100644 --- a/src/services/assembly/tiptapExtensions.test.ts +++ b/src/services/assembly/tiptapExtensions.test.ts @@ -10,7 +10,7 @@ import { WYSIWYG_COMPOSITION_ORDER } from "./compositionOrder"; describe("WI-3.4 — WYSIWYG composition order", () => { // 79 since audit 20260906 F5 added `safeBlockSplit` — Enter on a cross-block // selection, which StarterKit's splitBlock throws on. 80 since `undoIntegrity` - // stopped clearDocument rewriting an undo (#1407 soak, fuzz seed 4). + // stopped normalizers rewriting an undo (#1407 soak, fuzz seed 4). it("has 80 unique canonical entries", () => { expect(WYSIWYG_COMPOSITION_ORDER.length).toBe(80); expect(new Set(WYSIWYG_COMPOSITION_ORDER).size).toBe(80); diff --git a/src/services/assembly/tiptapExtensions.ts b/src/services/assembly/tiptapExtensions.ts index dc072a47e..7f1019e2f 100644 --- a/src/services/assembly/tiptapExtensions.ts +++ b/src/services/assembly/tiptapExtensions.ts @@ -161,8 +161,8 @@ export function buildExtensionList(config: TiptapExtensionConfig = {}): Extensio newGroupDelay: 500, }, }), - // Keeps Tiptap's clearDocument normalizer from rewriting an undo/redo, - // which corrupted the remaining undo history (plugins/undoIntegrity). + // Refuses any document change appended to an undo/redo, which corrupted + // the remaining undo history (plugins/undoIntegrity). undoIntegrityExtension, vmarkLinkExtension, // CJK-aware bold/italic (replaces StarterKit defaults) From 869ab12d92d49d3278e3b32edb7ef39a8e73ac03 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 14:10:53 +0800 Subject: [PATCH 10/11] test(gates): parse workflow expressions and follow env values through fallbacks Round 2 of the branch's cross-model audit found three more misses. check-workflow-input-fallbacks.test.mjs accepted any `||` after an input, so `format('{0}', inputs.seed, github.ref || '1')` passed with the seed still empty, and `github['event']['inputs']['seed']` was not recognized. It now reads each expression with a small parser for the Actions expression grammar and asks whether an empty input can reach the RESULT: `a || b` leaks what `b` leaks (or `a`, when `b` is itself empty), `&&` and function calls pass values through, comparisons and boolean functions never do. Property paths are resolved case-insensitively through dots and string keys, and an expression the parser cannot read is a finding, not a pass. check-numeric-env-reads.test.mjs only followed the left side of `??`, so `Number(override ?? process.env.FUZZ_SEED)` passed. It now follows a value through both sides of `??`, `||` and `&&`, string concatenation, conditional branches and templates, stops at a function call, and also counts arithmetic coercion (`x * 1`, `-x`, `x | 0`) as a conversion. Every new case has a self-test that failed first; both gates still report no findings on the repository. --- scripts/check-numeric-env-reads.test.mjs | 96 ++++++-- .../check-workflow-input-fallbacks.test.mjs | 209 ++++++++++++++---- 2 files changed, 244 insertions(+), 61 deletions(-) diff --git a/scripts/check-numeric-env-reads.test.mjs b/scripts/check-numeric-env-reads.test.mjs index c8fe90e19..21df14f69 100644 --- a/scripts/check-numeric-env-reads.test.mjs +++ b/scripts/check-numeric-env-reads.test.mjs @@ -11,7 +11,9 @@ * * It walks a TypeScript AST rather than grepping, so an explanation of the * defect in a comment or a string — like the one above — is prose, not a - * finding. It sees through parentheses, type assertions and `process["env"]`. + * finding. It sees through parentheses, type assertions, `process["env"]`, + * both sides of a fallback, conditionals and templates, and counts arithmetic + * coercion (`x * 1`, `-x`, `x | 0`) as a conversion. * Only files that mention `process` are parsed. Tracked and * untracked-not-ignored files are listed by `git ls-files`, so generated and * ignored trees cannot contribute. @@ -61,19 +63,68 @@ function dottedName(node) { return null; } -/** Whether `node` reads a variable off `process.env`, possibly with a `??`/`||` default. */ -function readsProcessEnv(node) { +/** Whether `node` is a direct `process.env.X` / `process.env["X"]` read. */ +function isEnvRead(node) { + if (!ts.isPropertyAccessExpression(node) && !ts.isElementAccessExpression(node)) return false; + const owner = dottedName(node.expression); + return owner === "process.env" || owner === "globalThis.process.env"; +} + +/** Operators that pass an operand's VALUE through, so an env read inside still decides the result. */ +const PASS_THROUGH = new Set([ + ts.SyntaxKind.QuestionQuestionToken, + ts.SyntaxKind.BarBarToken, + ts.SyntaxKind.AmpersandAmpersandToken, + ts.SyntaxKind.PlusToken, +]); + +/** + * Whether the value of `node` can be an environment string: a read itself, or + * one reachable through a fallback (either side of `??`, `||`, `&&`), string + * concatenation, a conditional branch, or a template. A function call is a + * boundary: whatever it returns is its own contract. + */ +function carriesEnvValue(node) { const current = unwrap(node); - if ( - ts.isBinaryExpression(current) && - (current.operatorToken.kind === ts.SyntaxKind.QuestionQuestionToken || - current.operatorToken.kind === ts.SyntaxKind.BarBarToken) - ) { - return readsProcessEnv(current.left); + if (isEnvRead(current)) return true; + if (ts.isBinaryExpression(current) && PASS_THROUGH.has(current.operatorToken.kind)) { + return carriesEnvValue(current.left) || carriesEnvValue(current.right); + } + if (ts.isConditionalExpression(current)) { + return carriesEnvValue(current.whenTrue) || carriesEnvValue(current.whenFalse); + } + if (ts.isTemplateExpression(current)) { + return current.templateSpans.some((span) => carriesEnvValue(span.expression)); + } + return false; +} + +/** Arithmetic operators that coerce an operand to a number, as `Number()` does. */ +const NUMERIC_BINARY = new Set([ + ts.SyntaxKind.MinusToken, + ts.SyntaxKind.AsteriskToken, + ts.SyntaxKind.SlashToken, + ts.SyntaxKind.PercentToken, + ts.SyntaxKind.AsteriskAsteriskToken, + ts.SyntaxKind.BarToken, + ts.SyntaxKind.AmpersandToken, + ts.SyntaxKind.CaretToken, + ts.SyntaxKind.LessThanLessThanToken, + ts.SyntaxKind.GreaterThanGreaterThanToken, + ts.SyntaxKind.GreaterThanGreaterThanGreaterThanToken, +]); +const NUMERIC_UNARY = new Set([ts.SyntaxKind.PlusToken, ts.SyntaxKind.MinusToken, ts.SyntaxKind.TildeToken]); + +/** Whether `node` converts an environment value to a number. */ +function convertsEnvToNumber(node) { + if (ts.isCallExpression(node)) { + return CONVERTERS.has(dottedName(node.expression) ?? "") && node.arguments.length > 0 && carriesEnvValue(node.arguments[0]); + } + if (ts.isPrefixUnaryExpression(node)) { + return NUMERIC_UNARY.has(node.operator) && carriesEnvValue(node.operand); } - if (ts.isPropertyAccessExpression(current) || ts.isElementAccessExpression(current)) { - const owner = dottedName(current.expression); - return owner === "process.env" || owner === "globalThis.process.env"; + if (ts.isBinaryExpression(node) && NUMERIC_BINARY.has(node.operatorToken.kind)) { + return carriesEnvValue(node.left) || carriesEnvValue(node.right); } return false; } @@ -92,15 +143,7 @@ function numericEnvReads(source, file) { const sourceFile = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, scriptKind(file)); const out = []; const visit = (node) => { - const converted = - (ts.isCallExpression(node) && - CONVERTERS.has(dottedName(node.expression) ?? "") && - node.arguments.length > 0 && - readsProcessEnv(node.arguments[0])) || - (ts.isPrefixUnaryExpression(node) && - node.operator === ts.SyntaxKind.PlusToken && - readsProcessEnv(node.operand)); - if (converted) { + if (convertsEnvToNumber(node)) { out.push(`${file}:${sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1}`); } ts.forEachChild(node, visit); @@ -151,6 +194,15 @@ describe("SELF-TEST: the detector", () => { "const c = Number(process.env.X as string);", "const d = Number(process.env.X!);", "const e = parseInt(process.env.X, 10);", + // Round 2 of the audit: a read on EITHER side of a fallback reaches the conversion. + "const f = Number(override ?? process.env.FUZZ_SEED);", + "const g = Number(flag && process.env.X);", + "const h = Number(cond ? process.env.X : 3);", + 'const i = Number(`${process.env.X}`);', + 'const j = Number("" + process.env.X);', + "const k = process.env.X * 1;", + "const l = -process.env.X;", + "const m = process.env.X | 0;", ])("flags %s", (line) => { expect(numericEnvReads(line, "x.ts")).toEqual(["x.ts:1"]); }); @@ -161,6 +213,8 @@ describe("SELF-TEST: the detector", () => { "const n = Number(settings.count);", '// the old shape: Number(process.env.FUZZ_SEED ?? "20260805")', 'const doc = "Number(process.env.X)";', + "const n = Number(parseEnv(process.env.X));", + "const o = Number(process.env.X === undefined);", ])("leaves %s alone", (line) => { expect(numericEnvReads(line, "x.ts")).toEqual([]); }); diff --git a/scripts/check-workflow-input-fallbacks.test.mjs b/scripts/check-workflow-input-fallbacks.test.mjs index 6d6049283..ef4e59825 100644 --- a/scripts/check-workflow-input-fallbacks.test.mjs +++ b/scripts/check-workflow-input-fallbacks.test.mjs @@ -22,6 +22,11 @@ * workflow-injection shape the repo's workflows already route through * `env:` to avoid. * + * Expressions are PARSED (a small reader for the expression grammar), not + * matched: a fallback must protect the input it follows, `github['event']` + * and `INPUTS.x` are the same read, a comparison is never empty, and an + * expression the reader cannot parse is a finding, never a pass. + * * `with:` is out of scope deliberately: actions read inputs through * `core.getInput`, which already treats "" as not provided. * @@ -43,45 +48,144 @@ const DIR = path.join(REPO, ".github/workflows"); const EXPRESSION = /\$\{\{([\s\S]*?)\}\}/g; const MARKER = /^\s*input-empty-ok:\s*(\S.*)?$/; -/** A single-quoted expression string; `''` is an escaped quote. */ -const STRING_LITERAL = /'(?:[^']|'')*'/g; -/** An input read, after string literals are masked: `inputs.x` or `inputs['x']`. */ -const INPUT_READ = /\b(?:github\.event\.)?inputs\s*(?:\.\s*[A-Za-z_][A-Za-z0-9_-]*|\[\s*@str\d+@\s*\])/g; -/** The first `||` operand after a read: a masked literal, or any other token. */ -const FALLBACK = /\|\|\s*(?:@str(\d+)@|[^\s|)]+)/; +const TOKEN = + /\s*(?:('(?:[^']|'')*')|(\d+(?:\.\d+)?|0x[0-9a-fA-F]+)|([A-Za-z_][A-Za-z0-9_-]*)|(\|\||&&|==|!=|<=|>=|[()[\].,!<>*]))/y; +/** Functions whose result is a boolean, and so never an empty string. */ +const BOOLEAN_FUNCTIONS = new Set(["contains", "startswith", "endswith", "success", "failure", "always", "cancelled"]); /** - * An expression body with each string literal replaced by `@str@`, so a - * literal that merely mentions `inputs.x` is not a read, and a bracket key - * (`inputs['x']`) or a fallback's emptiness can still be checked. + * Parse one GitHub Actions expression body into a small AST: + * `lit`, `ref` (a lowercase property path; a computed key becomes `*`), + * `call`, `or`, `and`, and `bool` for comparisons and negation. + * Throws on anything it cannot read, so an unparseable expression is a finding + * rather than a pass. */ -function maskStrings(body) { - const literals = []; - const masked = body.replace(STRING_LITERAL, (literal) => { - literals.push(literal.slice(1, -1).replace(/''/g, "'")); - return `@str${literals.length - 1}@`; - }); - return { masked, literals }; +function parseExpression(body) { + const tokens = []; + TOKEN.lastIndex = 0; + while (TOKEN.lastIndex < body.length && body.slice(TOKEN.lastIndex).trim() !== "") { + const at = TOKEN.lastIndex; + const m = TOKEN.exec(body); + if (!m) throw new Error(`unexpected character at ${at}`); + if (m[1] !== undefined) tokens.push({ kind: "str", value: m[1].slice(1, -1).replace(/''/g, "'") }); + else if (m[2] !== undefined) tokens.push({ kind: "num" }); + else if (m[3] !== undefined) tokens.push({ kind: "id", value: m[3].toLowerCase() }); + else tokens.push({ kind: m[4] }); + } + let i = 0; + const peek = (kind) => tokens[i]?.kind === kind; + const take = (kind) => { + if (!peek(kind)) throw new Error(`expected ${kind} at token ${i}`); + return tokens[i++]; + }; + const binary = (next, op, type) => () => { + let left = next(); + while (peek(op)) { + i++; + left = { type, left, right: next() }; + } + return left; + }; + const primary = () => { + if (peek("str")) return { type: "lit", value: take("str").value }; + if (peek("num")) return i++, { type: "lit", value: "0" }; + if (peek("(")) { + i++; + const inner = or(); + take(")"); + return inner; + } + const name = take("id").value; + if (name === "true" || name === "false") return { type: "bool" }; + if (name === "null") return { type: "lit", value: "" }; + if (peek("(")) { + i++; + const args = []; + while (!peek(")")) { + args.push(or()); + if (!peek(")")) take(","); + } + take(")"); + return BOOLEAN_FUNCTIONS.has(name) ? { type: "bool" } : { type: "call", args }; + } + let path = [name]; + for (;;) { + if (peek(".")) { + i++; + path.push(peek("*") ? (i++, "*") : take("id").value); + } else if (peek("[")) { + i++; + const key = or(); + take("]"); + path.push(key.type === "lit" ? key.value.toLowerCase() : "*"); + } else { + return { type: "ref", path }; + } + } + }; + const unary = () => (peek("!") ? (i++, unary(), { type: "bool" }) : primary()); + const compare = () => { + const left = unary(); + if (["==", "!=", "<", "<=", ">", ">="].some((op) => peek(op))) { + i++; + unary(); + return { type: "bool" }; + } + return left; + }; + const and = binary(compare, "&&", "and"); + const or = binary(and, "||", "or"); + const tree = or(); + if (i !== tokens.length) throw new Error(`unexpected token ${tokens[i].kind}`); + return tree; } -/** Every `${{ … }}` body in `text` that reads an input. */ -function inputExpressions(text) { - return [...text.matchAll(EXPRESSION)] - .map((match) => match[1]) - .filter((body) => maskStrings(body).masked.match(INPUT_READ) !== null); +/** Whether `node` is a read of a workflow input. */ +function isInputRead(node) { + if (node.type !== "ref") return false; + const [a, b, c] = node.path; + return a === "inputs" || (a === "github" && b === "event" && c === "inputs"); +} + +/** Whether `node` reads an input anywhere. */ +function readsInput(node) { + if (isInputRead(node)) return true; + if (node.type === "or" || node.type === "and") return readsInput(node.left) || readsInput(node.right); + if (node.type === "call") return node.args.some(readsInput); + return false; } /** - * Whether every input read in `body` is followed by a fallback that is not the - * empty string. A fallback that is itself an input read is checked in its own - * turn, so `inputs.a || inputs.b` still needs `inputs.b` to fall back. + * Whether `node` can evaluate to the empty string BECAUSE an input was empty. + * `a || b` is `b` whenever `a` is empty, so it leaks what `b` leaks — or what + * `a` leaks, if `b` is itself empty. `a && b` can be either operand. A call is + * assumed to pass an empty argument through; a comparison never does. */ -function everyReadHasFallback(body) { - const { masked, literals } = maskStrings(body); - return [...masked.matchAll(INPUT_READ)].every((read) => { - const fallback = FALLBACK.exec(masked.slice(read.index + read[0].length)); - if (fallback === null) return false; - return fallback[1] === undefined || literals[Number(fallback[1])] !== ""; +function leaksEmptyInput(node) { + switch (node.type) { + case "ref": + return isInputRead(node); + case "or": + return leaksEmptyInput(node.right) || (isEmptyLiteral(node.right) && leaksEmptyInput(node.left)); + case "and": + return leaksEmptyInput(node.left) || leaksEmptyInput(node.right); + case "call": + return node.args.some(leaksEmptyInput); + default: + return false; + } +} + +const isEmptyLiteral = (node) => node.type === "lit" && node.value === ""; + +/** Every `${{ … }}` body in `text`, parsed, or with the parse error. */ +function expressions(text) { + return [...text.matchAll(EXPRESSION)].map((match) => { + try { + return { body: match[1].trim(), tree: parseExpression(match[1]) }; + } catch (error) { + return { body: match[1].trim(), error: error instanceof Error ? error.message : String(error) }; + } }); } @@ -92,6 +196,8 @@ function everyReadHasFallback(body) { function findings(source, file) { const doc = parseDocument(source); const out = []; + const unparseable = (where, body, error) => + out.push({ where, problem: `cannot parse \`${body}\` (${error}); rewrite it or fix the checker` }); visit(doc, { Pair(_key, pair) { if (!isScalar(pair.key)) return; @@ -100,27 +206,31 @@ function findings(source, file) { if (key === "env" && isMap(pair.value)) { for (const entry of pair.value.items) { if (!isPair(entry) || !isScalar(entry.value) || typeof entry.value.value !== "string") continue; - const name = String(isScalar(entry.key) ? entry.key.value : "?"); - for (const body of inputExpressions(entry.value.value)) { - if (everyReadHasFallback(body)) continue; + const where = `${file} env ${String(isScalar(entry.key) ? entry.key.value : "?")}`; + for (const { body, tree, error } of expressions(entry.value.value)) { + if (error) { + unparseable(where, body, error); + continue; + } + if (!leaksEmptyInput(tree)) continue; const marker = MARKER.exec(entry.value.comment ?? ""); if (marker && marker[1]) continue; out.push({ - where: `${file} env ${name}`, + where, problem: marker ? "input-empty-ok marker has no reason" - : `reads \`${body.trim()}\` with no fallback and no input-empty-ok marker`, + : `reads \`${body}\` with no fallback and no input-empty-ok marker`, }); } } } if (key === "run" && isScalar(pair.value) && typeof pair.value.value === "string") { - for (const body of inputExpressions(pair.value.value)) { - out.push({ - where: `${file} run`, - problem: `interpolates \`${body.trim()}\` into the shell; pass it through env:`, - }); + for (const { body, tree, error } of expressions(pair.value.value)) { + if (error) unparseable(`${file} run`, body, error); + else if (readsInput(tree)) { + out.push({ where: `${file} run`, problem: `interpolates \`${body}\` into the shell; pass it through env:` }); + } } } }, @@ -194,6 +304,25 @@ describe("SELF-TEST: the checker", () => { expect(findings(wf(" S: ${{ inputs.a || inputs.b || 'c' }}"), "t.yml")).toEqual([]); }); + // Round 2 of the audit: a fallback must protect the input it follows. + it("does not accept a fallback that belongs to another operand", () => { + expect(findings(wf(" S: ${{ format('{0}', inputs.seed, github.ref || '1') }}"), "t.yml")).toHaveLength(1); + }); + + it("resolves a fully bracketed and case-varied input read", () => { + expect(findings(wf(" S: ${{ github['event']['inputs']['seed'] }}"), "t.yml")).toHaveLength(1); + expect(findings(wf(" S: ${{ INPUTS.seed }}"), "t.yml")).toHaveLength(1); + }); + + it("does not flag a comparison, which is never empty", () => { + expect(findings(wf(" S: ${{ inputs.seed == '' }}"), "t.yml")).toEqual([]); + }); + + it("refuses an expression it cannot parse instead of passing it", () => { + const out = findings(wf(" S: ${{ inputs.seed ||| 'x' }}"), "t.yml"); + expect(out.map((f) => f.problem)).toEqual([expect.stringMatching(/cannot parse/)]); + }); + it("ignores `inputs.x` inside a string literal", () => { expect(findings(wf(" S: ${{ format('see inputs.x') }}"), "t.yml")).toEqual([]); }); From 39d8b2f2e8d1e798e736dff6539a2c71398f1ef0 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Tue, 15 Sep 2026 14:29:19 +0800 Subject: [PATCH 11/11] fix: close the three gaps from the final audit round Round 3 of the branch's cross-model audit confirmed the round-2 fixes and found three more, each reproduced before fixing. - Footnote cleanup's "this document has no footnotes" cache went stale when it stood down on an undo that brought footnotes back: after replacing a document with plain text and undoing, deleting the restored reference left its definition orphaned (origin/main cleaned it up). A history batch now resets the cache before returning. The regression test fails without the reset. The plain two-statement form puts the file at 384 lines, so its size baseline returns to origin/main's 384; the branch's net change to that baseline is zero. - check-workflow-input-fallbacks.test.mjs treated only an empty literal as an empty fallback, so `inputs.seed || ('' || '')` and `inputs.seed || format('{0}', '')` passed. A fallback now protects an input only if it cannot evaluate to empty itself, decided recursively; a function call is assumed able to return "". - check-numeric-env-reads.test.mjs did not recognize `globalThis.Number`, `globalThis.Number.parseInt` or `window.parseFloat`. A global-object qualifier is now stripped before matching a converter. --- scripts/check-numeric-env-reads.test.mjs | 9 ++++- .../check-workflow-input-fallbacks.test.mjs | 33 +++++++++++++++++-- scripts/file-size-baseline.json | 2 +- src/plugins/footnotePopup/tiptap.ts | 9 +++-- src/plugins/undoIntegrity/tiptap.test.ts | 20 +++++++++++ 5 files changed, 65 insertions(+), 8 deletions(-) diff --git a/scripts/check-numeric-env-reads.test.mjs b/scripts/check-numeric-env-reads.test.mjs index 21df14f69..3c76acb76 100644 --- a/scripts/check-numeric-env-reads.test.mjs +++ b/scripts/check-numeric-env-reads.test.mjs @@ -115,10 +115,13 @@ const NUMERIC_BINARY = new Set([ ]); const NUMERIC_UNARY = new Set([ts.SyntaxKind.PlusToken, ts.SyntaxKind.MinusToken, ts.SyntaxKind.TildeToken]); +/** A converter's name with any global-object qualifier removed: `globalThis.Number` is `Number`. */ +const converterName = (callee) => (dottedName(callee) ?? "").replace(/^(?:globalThis|window|self|global)\./, ""); + /** Whether `node` converts an environment value to a number. */ function convertsEnvToNumber(node) { if (ts.isCallExpression(node)) { - return CONVERTERS.has(dottedName(node.expression) ?? "") && node.arguments.length > 0 && carriesEnvValue(node.arguments[0]); + return CONVERTERS.has(converterName(node.expression)) && node.arguments.length > 0 && carriesEnvValue(node.arguments[0]); } if (ts.isPrefixUnaryExpression(node)) { return NUMERIC_UNARY.has(node.operator) && carriesEnvValue(node.operand); @@ -203,6 +206,10 @@ describe("SELF-TEST: the detector", () => { "const k = process.env.X * 1;", "const l = -process.env.X;", "const m = process.env.X | 0;", + // Round 3 of the audit: a qualified built-in is the same converter. + "const q = globalThis.Number(process.env.N);", + "const r = globalThis.Number.parseInt(process.env.N, 10);", + "const t = window.parseFloat(process.env.N);", ])("flags %s", (line) => { expect(numericEnvReads(line, "x.ts")).toEqual(["x.ts:1"]); }); diff --git a/scripts/check-workflow-input-fallbacks.test.mjs b/scripts/check-workflow-input-fallbacks.test.mjs index ef4e59825..ceb1ead71 100644 --- a/scripts/check-workflow-input-fallbacks.test.mjs +++ b/scripts/check-workflow-input-fallbacks.test.mjs @@ -158,7 +158,7 @@ function readsInput(node) { /** * Whether `node` can evaluate to the empty string BECAUSE an input was empty. * `a || b` is `b` whenever `a` is empty, so it leaks what `b` leaks — or what - * `a` leaks, if `b` is itself empty. `a && b` can be either operand. A call is + * `a` leaks, if `b` can itself be empty. `a && b` can be either operand. A call is * assumed to pass an empty argument through; a comparison never does. */ function leaksEmptyInput(node) { @@ -166,7 +166,7 @@ function leaksEmptyInput(node) { case "ref": return isInputRead(node); case "or": - return leaksEmptyInput(node.right) || (isEmptyLiteral(node.right) && leaksEmptyInput(node.left)); + return leaksEmptyInput(node.right) || (mayBeEmpty(node.right) && leaksEmptyInput(node.left)); case "and": return leaksEmptyInput(node.left) || leaksEmptyInput(node.right); case "call": @@ -176,7 +176,27 @@ function leaksEmptyInput(node) { } } -const isEmptyLiteral = (node) => node.type === "lit" && node.value === ""; +/** + * Whether `node` can evaluate to the empty string at all, input or not: the + * test a fallback must fail to protect anything. A context read other than an + * input is assumed non-empty; a function call is assumed able to return "". + */ +function mayBeEmpty(node) { + switch (node.type) { + case "lit": + return node.value === ""; + case "ref": + return isInputRead(node); + case "or": + return mayBeEmpty(node.left) && mayBeEmpty(node.right); + case "and": + return mayBeEmpty(node.left) || mayBeEmpty(node.right); + case "call": + return true; + default: + return false; + } +} /** Every `${{ … }}` body in `text`, parsed, or with the parse error. */ function expressions(text) { @@ -309,6 +329,13 @@ describe("SELF-TEST: the checker", () => { expect(findings(wf(" S: ${{ format('{0}', inputs.seed, github.ref || '1') }}"), "t.yml")).toHaveLength(1); }); + // Round 3 of the audit: a fallback that can itself be empty protects nothing. + it("does not accept a fallback that can evaluate to empty", () => { + expect(findings(wf(" S: ${{ inputs.seed || ('' || '') }}"), "t.yml")).toHaveLength(1); + expect(findings(wf(" S: ${{ inputs.seed || format('{0}', '') }}"), "t.yml")).toHaveLength(1); + expect(findings(wf(" S: ${{ inputs.seed || github.ref }}"), "t.yml")).toEqual([]); + }); + it("resolves a fully bracketed and case-varied input read", () => { expect(findings(wf(" S: ${{ github['event']['inputs']['seed'] }}"), "t.yml")).toHaveLength(1); expect(findings(wf(" S: ${{ INPUTS.seed }}"), "t.yml")).toHaveLength(1); diff --git a/scripts/file-size-baseline.json b/scripts/file-size-baseline.json index 082733a1b..1a137bf4d 100644 --- a/scripts/file-size-baseline.json +++ b/scripts/file-size-baseline.json @@ -53,7 +53,7 @@ "src/plugins/codemirror/tableTabNav.ts": 385, "src/plugins/compositionGuard/tiptap.ts": 403, "src/plugins/editorPlugins.tiptap.ts": 320, - "src/plugins/footnotePopup/tiptap.ts": 381, + "src/plugins/footnotePopup/tiptap.ts": 384, "src/plugins/imagePasteToast/ImagePasteToastView.ts": 305, "src/plugins/latex/MathInlineNodeView.ts": 565, "src/plugins/mediaPopup/MediaPopupView.ts": 480, diff --git a/src/plugins/footnotePopup/tiptap.ts b/src/plugins/footnotePopup/tiptap.ts index d68246f7b..bb7383b83 100644 --- a/src/plugins/footnotePopup/tiptap.ts +++ b/src/plugins/footnotePopup/tiptap.ts @@ -303,9 +303,12 @@ export const footnotePopupExtension = Extension.create({ if (!refType || !defType) return null; const docChanged = transactions.some((tr) => tr.docChanged); - // An undo/redo restores recorded footnotes; cleanup would rewrite it and - // corrupt the undo history. A pending cleanup waits for the next edit. - if ((!docChanged && !cleanupPending) || isHistoryBatch(transactions)) return null; + if (!docChanged && !cleanupPending) return null; + // Undo/redo restores recorded footnotes: never rewrite it (see historyBatch), but the cache may be stale. + if (isHistoryBatch(transactions)) { + hasFootnotesCache = null; + return null; + } // Skip during IME composition — dispatching transactions mid-composition // can cause ProseMirror to reconcile the DOM, disrupting active CJK input diff --git a/src/plugins/undoIntegrity/tiptap.test.ts b/src/plugins/undoIntegrity/tiptap.test.ts index b3e97b75c..a03aca6f7 100644 --- a/src/plugins/undoIntegrity/tiptap.test.ts +++ b/src/plugins/undoIntegrity/tiptap.test.ts @@ -161,6 +161,26 @@ describe("normalizers on ordinary edits", () => { expect(blockTypes(session)).not.toContain("footnote_definition"); }); + // Standing down on an undo must not leave the plugin's "this document has no + // footnotes" cache stale when the undo brings footnotes back. Round 3 of the + // branch's audit. + it("still cleans up a definition after an undo restored its footnotes", () => { + session = createTypingSession({ markdown: "Text[^1].\n\n[^1]: note\n" }); + session.editor.view.dispatch(closeHistory(session.editor.state.tr)); + const { schema } = session.editor; + const plain = schema.nodes.paragraph.create(null, [schema.text("plain")]); + session.editor.view.dispatch(session.editor.state.tr.replaceWith(0, session.editor.state.doc.content.size, plain)); + session.editor.view.dispatch(closeHistory(session.editor.state.tr)); + session.undo(); + expect(blockTypes(session)).toContain("footnote_definition"); + let referenceAt = -1; + session.editor.state.doc.descendants((node, pos) => { + if (node.type.name === "footnote_reference") referenceAt = pos; + }); + session.editor.view.dispatch(session.editor.state.tr.delete(referenceAt, referenceAt + 1)); + expect(blockTypes(session)).not.toContain("footnote_definition"); + }); + it("still turns an emptied heading back into a paragraph", () => { session = createTypingSession({ markdown: "# Title\n" }); selectAllText(session);