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/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/scripts/check-numeric-env-reads.test.mjs b/scripts/check-numeric-env-reads.test.mjs new file mode 100644 index 000000000..3c76acb76 --- /dev/null +++ b/scripts/check-numeric-env-reads.test.mjs @@ -0,0 +1,228 @@ +/** + * 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. 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. + * + * @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"]); + +/** `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) { + 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` 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 (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]); + +/** 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(converterName(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.isBinaryExpression(node) && NUMERIC_BINARY.has(node.operatorToken.kind)) { + return carriesEnvValue(node.left) || carriesEnvValue(node.right); + } + 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) { + // 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) => { + if (convertsEnvToNumber(node)) { + 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);", + // 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);", + // 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;", + // 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"]); + }); + + 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)";', + "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 new file mode 100644 index 000000000..ceb1ead71 --- /dev/null +++ b/scripts/check-workflow-input-fallbacks.test.mjs @@ -0,0 +1,360 @@ +/** + * 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. + * + * 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. + * + * 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"); + +const EXPRESSION = /\$\{\{([\s\S]*?)\}\}/g; +const MARKER = /^\s*input-empty-ok:\s*(\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"]); + +/** + * 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 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; +} + +/** 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 `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` 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) { + switch (node.type) { + case "ref": + return isInputRead(node); + case "or": + return leaksEmptyInput(node.right) || (mayBeEmpty(node.right) && leaksEmptyInput(node.left)); + case "and": + return leaksEmptyInput(node.left) || leaksEmptyInput(node.right); + case "call": + return node.args.some(leaksEmptyInput); + default: + return false; + } +} + +/** + * 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) { + 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) }; + } + }); +} + +/** + * 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 = []; + 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; + 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 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, + problem: marker + ? "input-empty-ok marker has no reason" + : `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, 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:` }); + } + } + } + }, + }); + 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); + }); + + // 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([]); + }); + + // 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); + }); + + // 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); + }); + + 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([]); + }); + + it("ignores env values that read no input", () => { + expect(findings(wf(" GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}"), "t.yml")).toEqual([]); + }); +}); 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..bb7383b83 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"); @@ -309,6 +304,11 @@ export const footnotePopupExtension = Extension.create({ const docChanged = transactions.some((tr) => tr.docChanged); 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/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 new file mode 100644 index 000000000..a03aca6f7 --- /dev/null +++ b/src/plugins/undoIntegrity/tiptap.test.ts @@ -0,0 +1,190 @@ +/** + * Undo restores the document it recorded, and the undo history stays usable. + * + * 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: + * + * - 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, undo } from "@tiptap/pm/history"; +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 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 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 +// 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"); + }); + + // 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); + 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..5b604a52a --- /dev/null +++ b/src/plugins/undoIntegrity/tiptap.ts @@ -0,0 +1,89 @@ +/** + * 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. + * + * 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: + * - 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. + * + * 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 + */ +import { Extension } from "@tiptap/core"; +import { isHistoryTransaction } from "@tiptap/pm/history"; +import { Plugin, PluginKey, type Transaction } from "@tiptap/pm/state"; + +interface UndoIntegrityStorage { + /** The undo/redo transaction currently being applied, if any. */ + historyRoot: Transaction | null; +} + +const undoIntegrityKey = new PluginKey("undoIntegrity"); + +export const undoIntegrityExtension = Extension.create, UndoIntegrityStorage>({ + name: "undoIntegrity", + + addStorage() { + return { historyRoot: null }; + }, + + dispatchTransaction({ transaction, next }) { + const outer = this.storage.historyRoot; + this.storage.historyRoot = isHistoryTransaction(transaction) ? transaction : null; + try { + next(transaction); + } finally { + this.storage.historyRoot = outer; + } + }, + + addProseMirrorPlugins() { + const storage = this.storage; + return [ + new Plugin({ + key: undoIntegrityKey, + filterTransaction(transaction) { + const root = storage.historyRoot; + return root === null || transaction === root || !transaction.docChanged; + }, + }), + ]; + }, +}); 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..bc4e8a7b0 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 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); }); 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..7f1019e2f 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, }, }), + // 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) CJKBold, 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__/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/__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/__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/__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/__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 new file mode 100644 index 000000000..c9fa03f65 --- /dev/null +++ b/src/utils/markdownPipeline/listInterruptJoin.test.ts @@ -0,0 +1,128 @@ +// @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`); + }); + + // 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", () => { + 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/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", "> ", "- ", 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 58ccba313..1f05b111c 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 @@ -17,23 +19,30 @@ * 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 + * @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, repairSplitSurrogateEntities } from "./serializerStrikethrough"; +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"; 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"; @@ -70,13 +79,20 @@ 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, + // A text line ending that would make a blank line is written as a + // character reference (serializerText.ts). + text: handleText, ...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 @@ -112,12 +128,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.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..ad78ccd50 --- /dev/null +++ b/src/utils/markdownPipeline/serializerAttention.ts @@ -0,0 +1,191 @@ +/** + * 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`, 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 `~~`, + * 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/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; diff --git a/src/utils/markdownPipeline/serializerStrikethrough.ts b/src/utils/markdownPipeline/serializerStrikethrough.ts deleted file mode 100644 index 742e8456c..000000000 --- a/src/utils/markdownPipeline/serializerStrikethrough.ts +++ /dev/null @@ -1,177 +0,0 @@ -/** - * Strikethrough delimiter flanking — the `~~` half of what remark already does - * for `*`. - * - * Purpose: stop `~~` delimiters being emitted where GFM cannot parse them back. - * - * 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). - * - * `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 - * @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. - * - * `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); - }, - ) - ); -} 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); +}