From ba7623f81399e1161392a6b10b33d8e302a18061 Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Sat, 13 Jun 2026 08:41:31 -0700 Subject: [PATCH] #33: harden author-coloring PUA sentinels against intra-emphasis markdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The F9/F10 author-coloring technique injects paired PUA sentinels into prose source at span offsets, renders, then maps sentinels → cw-by-* spans. Two failure modes when a span boundary met emphasis markup (characterized in session 0032): - CASE1: a boundary strictly inside a delimiter run (a**b**c, between the two *) split `**`, breaking markdown-it's delimiter pairing (stray , raw **). - CASE3: a boundary inside an emphasis run (**bold** span covering **bo) rendered the emphasis but MISNESTED span/element (bold). Token-aware fix (both, per the issue #33 comment): - injectSentinels: clamp any sentinel offset that lands strictly inside a delimiter run (* _ ~ `) to the run's start, so a sentinel never splits a run (CASE1). Delimiters are invisible once rendered, so this only shifts the colored boundary across markup. Drop spans that clamp to empty. - sentinelsToSpans: replace the naive global split/join with a walker over the rendered HTML that emits the cw-by-* span only around TEXT runs — closing it before any and reopening after — so a span is always well-nested within inline elements (one span segment per text run, CASE3). Tags are copied verbatim with any stray sentinel stripped (no Private-Use-Area char leaks). Pure, vscode-free, deterministic (INV-33). No regression: existing colorByAuthor / renderReview cases stay green; the common no-emphasis case is byte-identical. 222 unit + 74/5 host E2E green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/trackChangesModel.ts | 94 ++++++++++++++++++++++++++++++---- test/trackChangesModel.test.ts | 39 ++++++++++++++ 2 files changed, 124 insertions(+), 9 deletions(-) diff --git a/src/trackChangesModel.ts b/src/trackChangesModel.ts index 32cc777..4235e60 100644 --- a/src/trackChangesModel.ts +++ b/src/trackChangesModel.ts @@ -545,13 +545,33 @@ function authorBadge(authors: Set): { cls: string; label: string } | : { cls: "cw-by-human", label: "You" }; } +// Markdown emphasis / code delimiters whose RUNS must never be split by an +// injected sentinel — a sentinel between two run chars (e.g. `*|*`) breaks +// markdown-it's delimiter pairing (#33 CASE1). +const isDelimChar = (c: string): boolean => c === "*" || c === "_" || c === "~" || c === "`"; + +/** + * #33 (CASE1): a sentinel must not land STRICTLY INSIDE a delimiter run. If `at` + * sits between two identical delimiter chars, snap it to the run's start (a + * position outside the run) so the run stays intact. Delimiters are invisible once + * rendered, so snapping only shifts the colored boundary across markup, never over + * visible text. + */ +function clampOffDelimiterRun(raw: string, at: number): number { + if (at <= 0 || at >= raw.length) return at; + if (!(isDelimChar(raw[at]) && raw[at - 1] === raw[at])) return at; + let p = at; + while (p > 0 && raw[p - 1] === raw[at]) p--; + return p; +} + /** Inject paired sentinels into a prose block's raw text for the spans clipped to it. */ function injectSentinels(raw: string, blockStart: number, spans: AuthorSpan[]): string { const inserts: { at: number; marker: string }[] = []; for (const s of spans) { - const lo = Math.max(0, s.start - blockStart); - const hi = Math.min(raw.length, s.end - blockStart); - if (hi <= lo) continue; + const lo = clampOffDelimiterRun(raw, Math.max(0, s.start - blockStart)); + const hi = clampOffDelimiterRun(raw, Math.min(raw.length, s.end - blockStart)); + if (hi <= lo) continue; // empty (or clamped to empty) span contributes nothing inserts.push({ at: lo, marker: SENT[s.author].open }); inserts.push({ at: hi, marker: SENT[s.author].close }); } @@ -564,13 +584,69 @@ function injectSentinels(raw: string, blockStart: number, spans: AuthorSpan[]): return out; } -/** Replace the rendered sentinels with author tags. */ +const SENTINEL_OF: Record = { + [SENT.claude.open]: { author: "claude", open: true }, + [SENT.claude.close]: { author: "claude", open: false }, + [SENT.human.open]: { author: "human", open: true }, + [SENT.human.close]: { author: "human", open: false }, +}; +const ALL_SENTINELS = new RegExp( + `[${SENT.claude.open}${SENT.claude.close}${SENT.human.open}${SENT.human.close}]`, + "g", +); + +/** + * #33: token-aware replacement of the rendered author sentinels with `cw-by-*` + * spans. A naive global string-replace (the old approach) could emit a span that + * CROSSES an element boundary — `bold` — when a + * boundary fell inside an emphasis run (CASE3). This walks the rendered HTML and + * emits the author span only around TEXT runs, CLOSING it before any `` and + * REOPENING it after, so a span is always well-nested within the inline elements + * (one `` segment per text run). Tags are copied verbatim (with any stray + * sentinel stripped, so no Private-Use-Area char ever leaks). Pure, deterministic. + */ function sentinelsToSpans(html: string): string { - return html - .split(SENT.claude.open).join('') - .split(SENT.claude.close).join("") - .split(SENT.human.open).join('') - .split(SENT.human.close).join(""); + const out: string[] = []; + let current: AuthorKind | null = null; // which author region we're inside + let spanOpen = false; // whether a is currently open in `out` + const openSpan = () => { + if (current && !spanOpen) { + out.push(``); + spanOpen = true; + } + }; + const closeSpan = () => { + if (spanOpen) { + out.push(""); + spanOpen = false; + } + }; + for (let i = 0; i < html.length; i++) { + const ch = html[i]; + const sentinel = SENTINEL_OF[ch]; + if (sentinel) { + if (sentinel.open) current = sentinel.author; // span opens lazily before the next text char + else { + closeSpan(); + current = null; + } + continue; + } + if (ch === "<") { + // An HTML tag: never let an author span straddle it (text `<` is escaped to + // <, so a raw `<` is always a real tag). Copy the tag verbatim, sentinel-free. + closeSpan(); + const gt = html.indexOf(">", i); + const end = gt === -1 ? html.length - 1 : gt; + out.push(html.slice(i, end + 1).replace(ALL_SENTINELS, "")); + i = end; + continue; + } + openSpan(); + out.push(ch); + } + closeSpan(); + return out.join(""); } /** diff --git a/test/trackChangesModel.test.ts b/test/trackChangesModel.test.ts index cb2e5c5..0b396fa 100644 --- a/test/trackChangesModel.test.ts +++ b/test/trackChangesModel.test.ts @@ -168,6 +168,45 @@ describe("colorByAuthor", () => { expect(html).toContain('hello'); expect(html).toContain("world"); }); + + // #33: author-coloring must be sentinel-safe around markdown emphasis. These + // exercise the REAL markdown-it renderer (via renderReview on an unchanged doc), + // since the failure is in markdown-it's inline parsing / element nesting. + // CASE1 — a span boundary strictly inside a delimiter run must not split it. + test("#33 CASE1: a span boundary inside ** does not break emphasis parsing", () => { + const doc = "a**b**c"; + const html = renderReview(doc, doc, [{ start: 0, end: 2, author: "human" }], []); + expect(html).toContain("b"); // emphasis still renders + expect(html).not.toContain("**"); // no raw delimiters left + expect(html).not.toContain(""); // no stray empty emphasis (the parse-break symptom) + expect(html).toContain('class="cw-by-human"'); // coloring present + }); + // CASE3 — a span boundary inside an emphasis run must color the text without + // misnesting span/element (the span is split at the element boundary). + test("#33 CASE3: a span boundary inside **bold** colors the text without misnesting", () => { + const doc = "**bold**"; + const html = renderReview(doc, doc, [{ start: 0, end: 4, author: "human" }], []); // covers "**bo" + expect(html).toContain(""); + expect(html).toContain('bold'); // span INSIDE strong, closed before "ld" + expect(html).not.toContain('cw-by-human">'); // NOT the old misnest (span wrapping the open) + }); + // CASE2 — a span covering a whole emphasis run stays correct (regression). + test("#33 CASE2: a span over the whole **bold** colors it correctly", () => { + const doc = "**bold**"; + const html = renderReview(doc, doc, [{ start: 0, end: 8, author: "human" }], []); + expect(html).toContain(""); + expect((html.match(/cw-by-human/g) ?? []).length).toBe(1); + expect(html).toContain("bold"); + }); + test("#33: no Private-Use-Area sentinel chars leak into the rendered output", () => { + const doc = "a**b**c and `co de` and _x_"; + const spans: AuthorSpan[] = [ + { start: 0, end: 2, author: "human" }, + { start: 12, end: 16, author: "claude" }, + ]; + const html = renderReview(doc, doc, spans, []); + expect(html).not.toMatch(/[\uE000-\uF8FF]/); // no leftover BMP Private-Use-Area sentinels + }); }); import { renderPlain } from "../src/trackChangesModel";