From 81f2da6baa85ed1f213619e51445ab9b09c41a15 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 20:52:06 +0000 Subject: [PATCH 1/3] fix(replace): tracked span inside another author's insertion splits it Word-style (#13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `replace --track` and `edit --at pN:S-E --track` share `replaceSpanInParagraph`. When the match lay wholly inside another author's pending ``, the replacement landed as a bare run inside that ins (credited to the other author), and when the match sat in a later run of the ins (e.g. after a nested ``) the offset walk started at that run and cut the wrong words. Add ONE early branch in `replaceSpanInParagraph` that dispatches to the new `core/find/replace-in-revision.tsx` when: the replace is tracked, the span is non-empty, every overlapping run shares one immediate parent, and that parent is a ``/`` directly under the paragraph. Another author's wrapper is split the way Word does it (wrapper keeps its id + pre text + the editor's nested `` of the cut; the editor's own top-level `` carries the replacement; the tail rides a fresh-id copy of the wrapper). The editor's own wrapper (exact `w:author` match) is rebuilt in place — same id, no ``, no split. Nested non-run children move whole at their view-visible length and keep their document position (one `` per contiguous cut group). Every other input takes the existing walkers unchanged: a golden comparison of 648 (fixture, command) pairs against main differs only on the 12 that hit the gate. No existing test expectation changed. Triage-Routine: true Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012GoxD36rnNfg6uArkVxtJL --- src/core/find/replace-in-revision.tsx | 172 ++++++++++++++++ src/core/find/replace-span.tsx | 29 ++- src/core/track-changes/CLAUDE.md | 6 +- tests/cli/edit.test.ts | 41 ++++ tests/cli/replace.test.ts | 272 +++++++++++++++++++++++++- 5 files changed, 517 insertions(+), 3 deletions(-) create mode 100644 src/core/find/replace-in-revision.tsx diff --git a/src/core/find/replace-in-revision.tsx b/src/core/find/replace-in-revision.tsx new file mode 100644 index 0000000..57473e4 --- /dev/null +++ b/src/core/find/replace-in-revision.tsx @@ -0,0 +1,172 @@ +import { runTextLength, sliceRun, wrapperContent, XmlNode } from "../parser"; +import { convertTextToDelText, type TrackedMeta } from "../track-changes"; +import { Del, Ins } from "../track-changes/emit"; +import type { FindView } from "./index"; +import { + isWrapperVisibleInView, + type ReplacementFormatting, + replacementRuns, + type Span, + sumVisibleTextLength, + type TrackedReplaceOptions, +} from "./replace-span"; + +/** + * Tracked replace of a span that lies WHOLLY inside another revision's + * `` / `` (issue #13). `replaceSpanInParagraph` dispatches + * here only when every overlapping run shares that wrapper as its immediate + * parent, the wrapper is a direct child of the paragraph, the span is + * non-empty, and the replace is tracked — every other input stays on the + * general walkers. + * + * Another author's wrapper is split the way Word does it: the wrapper keeps + * its id and the text before the span plus the cut, nested in the editor's + * `` (deleting inserted text is a del-of-ins); the editor's own + * `` carries the replacement at paragraph level; the text after the + * span rides a copy of the wrapper with a FRESH id. The editor's own wrapper + * is rebuilt in place instead (same id, same date) — the replacement is just + * more of that same insertion, so no split and no ``. + * + * Nested non-run children (another author's ``, bookmarks) move WHOLE, + * counted at their view-visible length, and are never wrapped in the new + * ``: one that falls inside the span keeps its place between the cut + * runs, which are deleted as one `` per contiguous group. + */ +export function replaceSpanInsideRevision( + paragraph: XmlNode, + wrapper: XmlNode, + span: Span, + replacement: string, + runProperties: XmlNode | null, + tracked: TrackedReplaceOptions, + view: FindView, + formatting?: ReplacementFormatting, +): void { + const wrapperIndex = paragraph.children.indexOf(wrapper); + const wrapperStart = sumVisibleTextLength( + paragraph.children.slice(0, wrapperIndex), + view, + ); + const parts = partitionWrapperChildren(wrapper, wrapperStart, span, view); + const runs = replacementRuns(runProperties, replacement, formatting); + + if (wrapper.attributes["w:author"] === tracked.meta.author) { + const markers = parts.cut.filter((child) => child.tag !== "w:r"); + wrapper.children = [...parts.pre, ...runs, ...markers, ...parts.post]; + return; + } + + wrapper.children = [...parts.pre, ...deleteCutInOrder(parts.cut, tracked)]; + const editorInsertion = {runs}; + const postHalf = + parts.post.length > 0 ? [postHalfOf(wrapper, parts.post, tracked)] : []; + paragraph.children.splice(wrapperIndex + 1, 0, editorInsertion, ...postHalf); +} + +/** Split the wrapper's children around `span` using the same offset space + * `collectRunSlots` walked: runs are sliced into pre / cut / post; any other + * child moves whole, counted at its visible length — one ending at or before + * the span start goes to `pre`, one starting at or after the span end to + * `post`, and one inside the span stays in `cut` at its document position. */ +function partitionWrapperChildren( + wrapper: XmlNode, + wrapperStart: number, + span: Span, + view: FindView, +): { pre: XmlNode[]; cut: XmlNode[]; post: XmlNode[] } { + const pre: XmlNode[] = []; + const cut: XmlNode[] = []; + const post: XmlNode[] = []; + let offset = wrapperStart; + + for (const child of wrapper.children) { + const length = + child.tag === "w:r" + ? runTextLength(child) + : isWrapperVisibleInView(child.tag, view) + ? sumVisibleTextLength(wrapperContent(child), view) + : 0; + const childStart = offset; + const childEnd = offset + length; + offset = childEnd; + + if (childEnd <= span.start) { + pre.push(child); + continue; + } + if (childStart >= span.end) { + post.push(child); + continue; + } + if (child.tag !== "w:r") { + cut.push(child); + continue; + } + + const sliceStartInRun = Math.max(0, span.start - childStart); + const sliceEndInRun = Math.min(length, span.end - childStart); + if (sliceStartInRun > 0) pre.push(sliceRun(child, 0, sliceStartInRun)); + cut.push(sliceRun(child, sliceStartInRun, sliceEndInRun)); + if (sliceEndInRun < length) { + post.push(sliceRun(child, sliceEndInRun, length)); + } + } + + return { pre, cut, post }; +} + +/** The cut, deleted in document order: each contiguous group of runs becomes + * one editor `` (text → delText); a non-run child between groups + * passes through untouched, where it was. */ +function deleteCutInOrder( + cut: XmlNode[], + tracked: TrackedReplaceOptions, +): XmlNode[] { + const out: XmlNode[] = []; + let group: XmlNode[] = []; + const flush = (): void => { + if (group.length === 0) return; + out.push({group}); + group = []; + }; + for (const child of cut) { + if (child.tag === "w:r") { + group.push(convertTextToDelText(child)); + continue; + } + flush(); + out.push(child); + } + flush(); + return out; +} + +/** The tail of a split wrapper: same tag, author and date, fresh `w:id` + * (two revisions may not share one). */ +function postHalfOf( + wrapper: XmlNode, + post: XmlNode[], + tracked: TrackedReplaceOptions, +): XmlNode { + const half = new XmlNode(wrapper.tag, { + ...wrapper.attributes, + "w:id": String(tracked.allocator.next()), + }); + half.children = post; + return half; +} + +function mintMeta(tracked: TrackedReplaceOptions): TrackedMeta { + return { ...tracked.meta, revisionId: tracked.allocator.next() }; +} + +/** The gate `replaceSpanInParagraph` evaluates: whether `node` is a + * `` / `` sitting directly under `paragraph`. A wrapper + * nested in a hyperlink, smartTag or field stays on the general walkers. */ +export function isDirectRevisionWrapper( + paragraph: XmlNode, + node: XmlNode, +): boolean { + if (node.tag !== "w:ins" && node.tag !== "w:moveTo") return false; + return paragraph.children.includes(node); +} diff --git a/src/core/find/replace-span.tsx b/src/core/find/replace-span.tsx index da44121..a91296f 100644 --- a/src/core/find/replace-span.tsx +++ b/src/core/find/replace-span.tsx @@ -14,6 +14,10 @@ import { import type { RevisionAllocator, TrackedMeta } from "../track-changes"; import { Del, Ins } from "../track-changes/emit"; import type { FindView } from "./index"; +import { + isDirectRevisionWrapper, + replaceSpanInsideRevision, +} from "./replace-in-revision"; export type Span = { start: number; end: number }; @@ -72,7 +76,9 @@ export function sumVisibleTextLength( * nodes converted to ), and the replacement is wrapped in * at the paragraph top level. When the replacement falls inside an * existing wrapper (same-parent case), it stays unwrapped and inherits the - * surrounding wrapper's attribution. + * surrounding wrapper's attribution — except a tracked replace wholly inside + * a paragraph-level /, which `replaceSpanInsideRevision` + * splits (another author) or rebuilds in place (the editor's own). */ export function replaceSpanInParagraph( paragraph: XmlNode, @@ -107,6 +113,27 @@ export function replaceSpanInParagraph( (slot) => slot.parent === firstParent, ); + // Tracked replace wholly inside another revision's / + // (issue #13): Word's split / same-author merge, in replace-in-revision. + if ( + tracked && + span.start < span.end && + allSameParent && + isDirectRevisionWrapper(paragraph, firstParent) + ) { + replaceSpanInsideRevision( + paragraph, + firstParent, + span, + replacement, + inheritedProperties, + tracked, + view, + formatting, + ); + return; + } + if (allSameParent) { const containerStart = firstParent === paragraph ? 0 : firstSlot.offsetBefore; diff --git a/src/core/track-changes/CLAUDE.md b/src/core/track-changes/CLAUDE.md index b6f70a5..04db3f0 100644 --- a/src/core/track-changes/CLAUDE.md +++ b/src/core/track-changes/CLAUDE.md @@ -62,7 +62,7 @@ The shorthand: - **Range edit** (M old → N new paragraphs): old 1..M-1 get content `` + paragraph-mark `` (accept-all merges them forward); the transition (old M) has its content ``'d, then the first new's content appended inline ``-wrapped, with paragraph-mark `` if N≥2; new 2..N each splice in as fresh `` with content `` and paragraph-mark `` (except the trailing one, whose mark is bare). - **Range delete** (M paragraphs): every paragraph in `[startIndex, endIndex]` gets content ``; paragraphs `startIndex..endIndex-1` get paragraph-mark ``. The last one's mark stays bare so accept-all leaves an empty residue paragraph (markdown render hides it). -**Re-editing an already-tracked paragraph — drop your own unaccepted insertions.** Every deletion-direction transform (`convertParagraphContentToDeleted` for the range paths, `applyDeletion` / `applyContentDeletion` for single delete) FIRST runs `dropOwnInsertions` (in [index.tsx](index.tsx)) over the paragraph's children: a top-level `` is content an earlier tracked edit in this same pass added but that hasn't been accepted, so re-deleting/replacing the paragraph must **un-insert** it (remove the wrapper + runs), not leave it visible. Without this, a paragraph edited twice under tracking (a single-block `--markdown` then a multi-block split — the contract-markup A-r1 shape — or an `edit` then a `delete`) kept the first edit's `` AND the second's, so Accept All showed the sentence twice and merged the following headings into a run-on. Dropping is correct on both sides of the review — accept and reject each make an unaccepted-then-replaced insertion vanish (reject restores the true baseline from the sibling ``). This assumes the doc-level single-author ("Reviewer") model; a genuine cross-author prior insertion would need Word's nested del-of-ins shape, which we don't model. `wrapContiguousTrackable` still passes existing `` / `` / `` / `` / `` wrappers through untouched — only additive `` is dropped, and only in the deletion direction (never in `applyInsertion` / `wrapNewParagraphContentAsInserted`, which build or preserve NEW insertions, e.g. CriticMarkup from `edit --markdown`). +**Re-editing an already-tracked paragraph — drop your own unaccepted insertions.** Every deletion-direction transform (`convertParagraphContentToDeleted` for the range paths, `applyDeletion` / `applyContentDeletion` for single delete) FIRST runs `dropOwnInsertions` (in [index.tsx](index.tsx)) over the paragraph's children: a top-level `` is content an earlier tracked edit in this same pass added but that hasn't been accepted, so re-deleting/replacing the paragraph must **un-insert** it (remove the wrapper + runs), not leave it visible. Without this, a paragraph edited twice under tracking (a single-block `--markdown` then a multi-block split — the contract-markup A-r1 shape — or an `edit` then a `delete`) kept the first edit's `` AND the second's, so Accept All showed the sentence twice and merged the following headings into a run-on. Dropping is correct on both sides of the review — accept and reject each make an unaccepted-then-replaced insertion vanish (reject restores the true baseline from the sibling ``). This assumes the doc-level single-author ("Reviewer") model; a genuine cross-author prior insertion would need Word's nested del-of-ins shape, which only the span replace models (see "Tracked span replace inside another author's insertion" below), not these whole-paragraph transforms. `wrapContiguousTrackable` still passes existing `` / `` / `` / `` / `` wrappers through untouched — only additive `` is dropped, and only in the deletion direction (never in `applyInsertion` / `wrapNewParagraphContentAsInserted`, which build or preserve NEW insertions, e.g. CriticMarkup from `edit --markdown`). ## The shared range guard @@ -81,6 +81,10 @@ The shorthand: rPr equality is structural (XML-string compare on cloned nodes). The codebase's emitter produces canonical output so this works in practice; non-canonical rPr from an external producer might leave equivalent rPr blocks unmerged (each token gets its own run). Acceptable. +## Tracked span replace inside another author's insertion (issue #13) + +`replaceSpanInParagraph` (the engine behind `replace --track` and `edit --at pN:S-E --track`) has ONE early branch, dispatching to [core/find/replace-in-revision.tsx](../find/replace-in-revision.tsx). **The gate**: the replace is tracked, the span is non-empty, every overlapping run shares the same immediate parent, and that parent is a `` / `` sitting DIRECTLY under the paragraph. Everything else — untracked, empty spans, a span crossing the wrapper's edge, a wrapper nested in a hyperlink/smartTag/field, a match inside a `` under `--current`/`--baseline` — stays on the general walkers, byte-for-byte as before. Inside the gate, `w:author` decides (exact string match): **another author's wrapper is split the way Word does it** — the wrapper keeps its `w:id` plus the text before the span and the editor's `` of the cut nested inside it (a del-of-ins, so rejecting either author's revision restores the right text), the replacement rides the editor's own paragraph-level ``, and the text after the span rides a copy of the wrapper with a FRESH id from the shared allocator (no post-half when the span ends at the wrapper's end). **The editor's own wrapper is rebuilt in place** (same id, same date, no ``, no split) — the replacement is just more of that same insertion. The new path computes the wrapper's start offset from the siblings before it, where the general `rebuildContainer` starts its walk at the first OVERLAPPING run's offset — correct only when the match sits in the wrapper's first run, which is the "deleted the wrong words" half of the bug (the nested `` in the issue is just what put the match in a later run; the same miscount still applies to an untracked multi-run wrapper and to hyperlinks, both deliberately untouched here). Nested non-run children (a third author's ``, bookmarks) move WHOLE, counted at their view-visible length (a `` is 0 wide in the accepted view), and are never wrapped in the new ``: one inside the span keeps its place, so the cut is deleted as one `` per contiguous run group. Known limitations, all deliberately left on the old paths: an insertion nested in a hyperlink still gets a bare run inside it; an empty span (`pN:S-S`) inserts a bare run inside the other author's wrapper; a span crossing out of the wrapper takes `rebuildAcrossBoundaries`, whose two halves still share the original `w:id`; a `--current` match inside another author's `` still lands the replacement inside that deletion. The `track-changes list` text table pairs an adjacent del+ins into a `revN` without checking authors, so a split like this reads as `rev0 replace "cut" → ""`; `--json` carries each revision's author. + ## Adding a new tracked-change kind See [cli/track-changes/CLAUDE.md "Adding a tracked-change kind"](../../cli/track-changes/CLAUDE.md). The summary: extend `TrackedChange["kind"]` in `core/ast/types.ts`, widen `cli/info/schema.ts`, register in `walkRunContainer` in `core/ast/read.ts` (the single source of `tcN` ids), and handle in `actionFor` / `applyAccept` / `applyReject` here. If the wrapper tag is ambiguous (table-structural, checkboxToggle), set `reference.kind` explicitly in the reader so `changeFoundFromReference` doesn't have to guess; otherwise `trackedChangeKindForTag` covers it. Surface in `paragraphTextAccepted` / `paragraphTextBaseline` ([core/ast/text.ts](../ast/text.ts)) and `cli/read/markdown.ts` if the kind affects visible text. diff --git a/tests/cli/edit.test.ts b/tests/cli/edit.test.ts index f4ba525..b868207 100644 --- a/tests/cli/edit.test.ts +++ b/tests/cli/edit.test.ts @@ -719,6 +719,47 @@ describe("docx edit --at pN:S-E — character-span edit", () => { expect(changes.every((change) => change.author === "Reviewer")).toBe(true); }); + test("tracked span inside another author's insertion splits it around the editor's del + ins (#13)", async () => { + // Same engine as `replace`: the third author's nested stays in + // place and must not shift the cut; the replacement must not land bare + // inside Reviewer A's insertion; A's tail gets a fresh id. + const docPath = await buildRawDoc( + 'Fees are due. The Client may withhold any disputed amount.', + "span-in-ins", + ); + const locator = await firstLocator(docPath, "disputed amount"); + expect(locator).toBe("p0:38-53"); + const result = await runCli( + "edit", + docPath, + "--at", + locator, + "--text", + "disputed sum", + "--track", + "--author", + "Editor", + ); + expect(result.exitCode).toBe(0); + + const xml = (await readDocumentXml(docPath)).replaceAll( + /(w:author="Editor") w:date="[^"]*"/g, + '$1 w:date="NOW"', + ); + expect(xml).toContain( + 'Fees are due. ' + + 'The Client may withhold ' + + 'any ' + + 'disputed amount' + + 'disputed sum' + + '.', + ); + const accepted = await runCli("read", docPath, "--accepted"); + expect(accepted.stdout).toContain( + "Fees are due. The Client may withhold disputed sum.", + ); + }); + test("out-of-range span fails with INVALID_LOCATOR", async () => { const docPath = await freshSpan("span-oob"); const result = await runCli( diff --git a/tests/cli/replace.test.ts b/tests/cli/replace.test.ts index e2dc7f4..119253f 100644 --- a/tests/cli/replace.test.ts +++ b/tests/cli/replace.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { join } from "node:path"; import { runCli, tempWorkspace } from "./harness"; -import { readDocumentXml } from "./helpers"; +import { buildRawDoc, readDocumentXml } from "./helpers"; type Body = { blocks: Array<{ @@ -1201,3 +1201,273 @@ test("replace and replace batch apply complex-script override only to replacemen ).exitCode, ).toBe(2); }); + +// Issue #13: a tracked replace whose match lies WHOLLY inside another author's +// pending . Main put the replacement as a bare run inside that ins +// (credited to the other author) and, when the match sat in a later run of the +// ins (here: after a nested ), started its offset walk at that run's +// offset and cut the wrong words. Word's shape: the other author's ins keeps +// its id and the text before the match plus the editor's nested of +// the cut; the replacement rides the editor's own top-level ; the text +// after the match rides a copy of the ins with a fresh id. When the editor IS +// that author the ins is rebuilt in place (no split, no ). +describe("docx replace --track inside another author's insertion (#13)", () => { + const INS_A = + ''; + const SINGLE_RUN = `The Client ${INS_A}may withhold a disputed amount.`; + const NESTED_DEL = `Fees are due. ${INS_A}The Client may withhold any disputed amount.`; + + async function trackedReplace( + body: string, + label: string, + author: string, + pattern = "disputed amount", + ...flags: string[] + ): Promise { + const docPath = await buildRawDoc(body, label); + const result = await runCli( + "replace", + docPath, + pattern, + "disputed sum", + "--track", + "--author", + author, + ...flags, + ); + expect(result.exitCode).toBe(0); + // Every emitted shape must pass the ECMA-376 schema gate Word applies. + expect((await runCli("validate", docPath)).exitCode).toBe(0); + return docPath; + } + + /** The first `` of document.xml with the editor's (now) dates pinned, + * so the whole revision structure can be asserted as one string. */ + async function paragraphXml( + docPath: string, + editor: string, + ): Promise { + const xml = await readDocumentXml(docPath); + const paragraph = xml.match(/.*?<\/w:p>/s)?.[0] ?? ""; + return paragraph.replaceAll( + new RegExp(`(w:author="${editor}") w:date="[^"]*"`, "g"), + '$1 w:date="NOW"', + ); + } + + async function viewText(docPath: string, view: string): Promise { + const result = await runCli("read", docPath, view); + return result.stdout + .split("\n") + .filter((line) => line.trim() && !/^$/.test(line.trim())) + .map((line) => line.replace(/\s*\s*$/, "")) + .join("\n"); + } + + async function listChanges( + docPath: string, + ): Promise< + Array<{ id: string; kind: string; text: string; author: string }> + > { + const result = await runCli("track-changes", "list", docPath); + return result.parsed as Array<{ + id: string; + kind: string; + text: string; + author: string; + }>; + } + + test("case 1: the other author's ins is split around the editor's del + ins, tail gets a fresh id", async () => { + const docPath = await trackedReplace(SINGLE_RUN, "ins-split", "Editor"); + expect(await paragraphXml(docPath, "Editor")).toBe( + 'The Client ' + + `${INS_A}may withhold a ` + + 'disputed amount' + + 'disputed sum' + + '.', + ); + expect(await viewText(docPath, "--accepted")).toBe( + "The Client may withhold a disputed sum.", + ); + + const changes = await listChanges(docPath); + expect( + changes.map((change) => [change.kind, change.author, change.text]), + ).toEqual([ + ["ins", "Reviewer A", "may withhold a "], + ["del", "Editor", "disputed amount"], + ["ins", "Editor", "disputed sum"], + ["ins", "Reviewer A", "."], + ]); + }); + + test("case 2: a nested deletion by a third author stays put and does not shift the cut", async () => { + const docPath = await trackedReplace( + NESTED_DEL, + "ins-nested-del", + "Editor", + ); + expect(await paragraphXml(docPath, "Editor")).toBe( + 'Fees are due. ' + + `${INS_A}The Client may withhold ` + + 'any ' + + 'disputed amount' + + 'disputed sum' + + '.', + ); + expect(await viewText(docPath, "--accepted")).toBe( + "Fees are due. The Client may withhold disputed sum.", + ); + const changes = await listChanges(docPath); + expect(changes.map((change) => [change.kind, change.author])).toEqual([ + ["ins", "Reviewer A"], + ["del", "Reviewer B"], + ["del", "Editor"], + ["ins", "Editor"], + ["ins", "Reviewer A"], + ]); + }); + + test("case 3: the author's OWN ins is rebuilt in place — same id, no del, no split", async () => { + const docPath = await trackedReplace(SINGLE_RUN, "ins-own", "Reviewer A"); + // Nothing to pin: no Editor revision exists, and A's date must survive. + expect(await paragraphXml(docPath, "Editor")).toBe( + 'The Client ' + + '' + + 'may withhold a ' + + 'disputed sum' + + '.', + ); + expect(await viewText(docPath, "--accepted")).toBe( + "The Client may withhold a disputed sum.", + ); + expect(await listChanges(docPath)).toHaveLength(1); + }); + + test("the author match is exact: 'Reviewer a' is another author", async () => { + const docPath = await trackedReplace(SINGLE_RUN, "ins-case", "Reviewer a"); + const xml = await readDocumentXml(docPath); + expect(xml).toContain(' { + const docPath = await trackedReplace(SINGLE_RUN, "ins-reject", "Editor"); + const editorIds = (await listChanges(docPath)) + .filter((change) => change.author === "Editor") + .map((change) => change.id); + expect(editorIds).toHaveLength(2); + const rejected = await runCli( + "track-changes", + "reject", + docPath, + ...editorIds.flatMap((id) => ["--at", id]), + ); + expect(rejected.exitCode).toBe(0); + + expect(await viewText(docPath, "--accepted")).toBe( + "The Client may withhold a disputed amount.", + ); + expect(await viewText(docPath, "--baseline")).toBe("The Client"); + const xml = await readDocumentXml(docPath); + expect(xml).not.toContain('w:author="Editor"'); + expect(xml).toContain( + `${INS_A}may withhold a disputed amount`, + ); + expect((await listChanges(docPath)).map((change) => change.text)).toEqual([ + "may withhold a disputed amount", + ".", + ]); + }); + + test("--all: two matches inside the same ins split it twice, right to left", async () => { + const docPath = await trackedReplace( + `${INS_A}disputed amount beta disputed amount.`, + "ins-all", + "Editor", + "disputed amount", + "--all", + ); + expect(await viewText(docPath, "--accepted")).toBe( + "disputed sum beta disputed sum.", + ); + const changes = await listChanges(docPath); + // The head of A's ins holds only the editor's nested del, so the reader + // lists it with no visible text of its own. + expect( + changes.map((change) => [change.kind, change.author, change.text]), + ).toEqual([ + ["ins", "Reviewer A", ""], + ["del", "Editor", "disputed amount"], + ["ins", "Editor", "disputed sum"], + ["ins", "Reviewer A", " beta "], + ["del", "Editor", "disputed amount"], + ["ins", "Editor", "disputed sum"], + ["ins", "Reviewer A", "."], + ]); + const ids = [ + ...(await readDocumentXml(docPath)).matchAll(/w:id="(\d+)"/g), + ].map((match) => match[1]); + expect(new Set(ids).size).toBe(ids.length); + }); + + test("a third author's deletion INSIDE the match keeps its place between two editor dels", async () => { + const docPath = await trackedReplace( + `${INS_A}pay disputed old amount now.`, + "ins-del-inside", + "Editor", + ); + expect(await paragraphXml(docPath, "Editor")).toBe( + `${INS_A}pay ` + + 'disputed ' + + 'old ' + + 'amount' + + 'disputed sum' + + ' now.', + ); + expect(await viewText(docPath, "--accepted")).toBe("pay disputed sum now."); + // Current view keeps the original word order of the deleted text. + const current = await runCli("read", docPath, "--current"); + expect(current.stdout.replaceAll(/\[\^tc\d+\]/g, "")).toContain( + "{++pay ++}{--disputed --}{--old --}{--amount--}{++disputed sum++}{++ now.++}", + ); + }); + + test("a span CROSSING out of the ins keeps the general walker's shape", async () => { + // Only a match wholly inside the wrapper is gated; this one starts in the + // plain run before it. The expected string is main's output, pinned + // verbatim (its reuse of w:id="1" on both halves is pre-existing). + const docPath = await trackedReplace( + SINGLE_RUN, + "ins-crossing", + "Editor", + "Client may", + ); + expect(await paragraphXml(docPath, "Editor")).toBe( + 'The ' + + 'Client ' + + 'disputed sum' + + `${INS_A}may` + + `${INS_A} withhold a disputed amount.`, + ); + }); + + test("untracked, the replacement lands inside the ins exactly as before", async () => { + const docPath = await buildRawDoc(SINGLE_RUN, "ins-untracked"); + const result = await runCli( + "replace", + docPath, + "disputed amount", + "disputed sum", + ); + expect(result.exitCode).toBe(0); + expect(await paragraphXml(docPath, "Editor")).toBe( + 'The Client ' + + `${INS_A}may withhold a ` + + 'disputed sum' + + '.', + ); + }); +}); From fd39a919e77215bf15771efe018292410db04f06 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 21:29:38 +0000 Subject: [PATCH 2/3] fix(replace): keep anchors, split own moveTo, no empty tail revision, per-author revN (#13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups on the in-revision span replace: - The editor's own `` rebuild no longer discards the cut runs outright: every non-text child (footnote/endnote/comment reference, drawing, text-box anchor, object) is kept in place, so what was anchored on the replaced words survives (`cutRemnants`). - The in-place merge is restricted to ``. A same-author `` takes the split path — moved text is relocated original text, not the author's new words. - A tail that holds only markers (comment range end + its reference run, bookmark end) stays in the wrapper after the cut instead of riding a fresh-id copy that would list as an empty revision (`isPureMarker`). - `revN` pairing (`cli/track-changes/groups.ts`) requires both halves to share an author, so the other author's head ins never pairs with the editor's nested del. `track-changes list` output is unchanged on every fixture. Triage-Routine: true Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012GoxD36rnNfg6uArkVxtJL --- src/cli/track-changes/groups.ts | 11 +- src/core/find/replace-in-revision.tsx | 85 +++++++++-- src/core/track-changes/CLAUDE.md | 2 +- tests/cli/replace.test.ts | 207 +++++++++++++++++++++++++- 4 files changed, 287 insertions(+), 18 deletions(-) diff --git a/src/cli/track-changes/groups.ts b/src/cli/track-changes/groups.ts index 6a07571..3eeebb2 100644 --- a/src/cli/track-changes/groups.ts +++ b/src/cli/track-changes/groups.ts @@ -9,7 +9,12 @@ * maps `revN` ⇄ its member `tcN`s, computed the SAME way in `list` and `apply` * (sorted by tcN index, then paired) so the two can't disagree. */ -type Groupable = { id: string; kind: string; blockId?: string }; +type Groupable = { + id: string; + kind: string; + blockId?: string; + author: string; +}; export type RevisionGroups = { /** `revN` → its member `tcN` ids (always exactly the two halves of a replace). */ @@ -47,9 +52,11 @@ export function revisionGroups(changes: Groupable[]): RevisionGroups { * "replace" shape. Restricted to plain `ins`/`del` (not moveFrom/moveTo, table, * or section-property revisions) so grouping stays conservative; same-paragraph * ins+ins / del+del (a paragraph insert/delete's run + paragraph-mark markers) - * never pair. */ + * never pair. Both halves must share an author: one author's deletion nested + * in another's insertion (a cross-author span replace) is two decisions. */ function isTextReplacePair(a: Groupable, b: Groupable): boolean { if (a.blockId === undefined || a.blockId !== b.blockId) return false; + if (a.author !== b.author) return false; const pair = `${a.kind}+${b.kind}`; return pair === "del+ins" || pair === "ins+del"; } diff --git a/src/core/find/replace-in-revision.tsx b/src/core/find/replace-in-revision.tsx index 57473e4..c9bb63e 100644 --- a/src/core/find/replace-in-revision.tsx +++ b/src/core/find/replace-in-revision.tsx @@ -19,13 +19,19 @@ import { * non-empty, and the replace is tracked — every other input stays on the * general walkers. * - * Another author's wrapper is split the way Word does it: the wrapper keeps - * its id and the text before the span plus the cut, nested in the editor's - * `` (deleting inserted text is a del-of-ins); the editor's own - * `` carries the replacement at paragraph level; the text after the - * span rides a copy of the wrapper with a FRESH id. The editor's own wrapper - * is rebuilt in place instead (same id, same date) — the replacement is just - * more of that same insertion, so no split and no ``. + * The wrapper is split the way Word does it: it keeps its id and the text + * before the span plus the cut, nested in the editor's `` (deleting + * inserted text is a del-of-ins); the editor's own `` carries the + * replacement at paragraph level; the content after the span rides a copy of + * the wrapper with a FRESH id — unless that tail is nothing but markers (a + * comment range end and its reference run, a bookmark end), which stay in + * the wrapper after the cut rather than becoming an empty revision. The one + * exception to splitting is the editor's OWN ``, rebuilt in place + * (same id, same date): the replacement is just more of that same insertion, + * so no split and no `` — though every non-text child of the cut (a + * note or comment reference, a drawing, a text-box anchor) is kept. A + * `` is never merged into, even by its own author: moved text is + * relocated original text, not the author's new words. * * Nested non-run children (another author's ``, bookmarks) move WHOLE, * counted at their view-visible length, and are never wrapped in the new @@ -50,17 +56,28 @@ export function replaceSpanInsideRevision( const parts = partitionWrapperChildren(wrapper, wrapperStart, span, view); const runs = replacementRuns(runProperties, replacement, formatting); - if (wrapper.attributes["w:author"] === tracked.meta.author) { - const markers = parts.cut.filter((child) => child.tag !== "w:r"); - wrapper.children = [...parts.pre, ...runs, ...markers, ...parts.post]; + if ( + wrapper.tag === "w:ins" && + wrapper.attributes["w:author"] === tracked.meta.author + ) { + wrapper.children = [ + ...parts.pre, + ...runs, + ...cutRemnants(parts.cut), + ...parts.post, + ]; return; } - wrapper.children = [...parts.pre, ...deleteCutInOrder(parts.cut, tracked)]; + const markerTail = parts.post.every(isPureMarker); + wrapper.children = [ + ...parts.pre, + ...deleteCutInOrder(parts.cut, tracked), + ...(markerTail ? parts.post : []), + ]; const editorInsertion = {runs}; - const postHalf = - parts.post.length > 0 ? [postHalfOf(wrapper, parts.post, tracked)] : []; - paragraph.children.splice(wrapperIndex + 1, 0, editorInsertion, ...postHalf); + const tail = markerTail ? [] : [postHalfOf(wrapper, parts.post, tracked)]; + paragraph.children.splice(wrapperIndex + 1, 0, editorInsertion, ...tail); } /** Split the wrapper's children around `span` using the same offset space @@ -115,6 +132,26 @@ function partitionWrapperChildren( return { pre, cut, post }; } +/** What the editor's own cut leaves behind, in order: every non-run child, + * and each cut run stripped of its text — a run that still carries a child + * beyond `` (a note or comment reference, a drawing, a text-box + * anchor) stays, so whatever was anchored on the replaced words survives; + * a text-only run is dropped. */ +function cutRemnants(cut: XmlNode[]): XmlNode[] { + const out: XmlNode[] = []; + for (const child of cut) { + if (child.tag !== "w:r") { + out.push(child); + continue; + } + child.children = child.children.filter( + (part) => part.tag !== "w:t" && part.tag !== "w:delText", + ); + if (child.children.some((part) => part.tag !== "w:rPr")) out.push(child); + } + return out; +} + /** The cut, deleted in document order: each contiguous group of runs becomes * one editor `` (text → delText); a non-run child between groups * passes through untouched, where it was. */ @@ -141,6 +178,26 @@ function deleteCutInOrder( return out; } +const PURE_MARKER_TAGS: ReadonlySet = new Set([ + "w:bookmarkStart", + "w:bookmarkEnd", + "w:commentRangeStart", + "w:commentRangeEnd", + "w:permStart", + "w:permEnd", + "w:proofErr", +]); + +/** A range/annotation marker that is not revision content: one of the + * paragraph-level marker tags, or a run holding only a comment reference. */ +function isPureMarker(node: XmlNode): boolean { + if (PURE_MARKER_TAGS.has(node.tag)) return true; + if (node.tag !== "w:r") return false; + return node.children.every( + (part) => part.tag === "w:rPr" || part.tag === "w:commentReference", + ); +} + /** The tail of a split wrapper: same tag, author and date, fresh `w:id` * (two revisions may not share one). */ function postHalfOf( diff --git a/src/core/track-changes/CLAUDE.md b/src/core/track-changes/CLAUDE.md index 04db3f0..1aebcb1 100644 --- a/src/core/track-changes/CLAUDE.md +++ b/src/core/track-changes/CLAUDE.md @@ -83,7 +83,7 @@ rPr equality is structural (XML-string compare on cloned nodes). The codebase's ## Tracked span replace inside another author's insertion (issue #13) -`replaceSpanInParagraph` (the engine behind `replace --track` and `edit --at pN:S-E --track`) has ONE early branch, dispatching to [core/find/replace-in-revision.tsx](../find/replace-in-revision.tsx). **The gate**: the replace is tracked, the span is non-empty, every overlapping run shares the same immediate parent, and that parent is a `` / `` sitting DIRECTLY under the paragraph. Everything else — untracked, empty spans, a span crossing the wrapper's edge, a wrapper nested in a hyperlink/smartTag/field, a match inside a `` under `--current`/`--baseline` — stays on the general walkers, byte-for-byte as before. Inside the gate, `w:author` decides (exact string match): **another author's wrapper is split the way Word does it** — the wrapper keeps its `w:id` plus the text before the span and the editor's `` of the cut nested inside it (a del-of-ins, so rejecting either author's revision restores the right text), the replacement rides the editor's own paragraph-level ``, and the text after the span rides a copy of the wrapper with a FRESH id from the shared allocator (no post-half when the span ends at the wrapper's end). **The editor's own wrapper is rebuilt in place** (same id, same date, no ``, no split) — the replacement is just more of that same insertion. The new path computes the wrapper's start offset from the siblings before it, where the general `rebuildContainer` starts its walk at the first OVERLAPPING run's offset — correct only when the match sits in the wrapper's first run, which is the "deleted the wrong words" half of the bug (the nested `` in the issue is just what put the match in a later run; the same miscount still applies to an untracked multi-run wrapper and to hyperlinks, both deliberately untouched here). Nested non-run children (a third author's ``, bookmarks) move WHOLE, counted at their view-visible length (a `` is 0 wide in the accepted view), and are never wrapped in the new ``: one inside the span keeps its place, so the cut is deleted as one `` per contiguous run group. Known limitations, all deliberately left on the old paths: an insertion nested in a hyperlink still gets a bare run inside it; an empty span (`pN:S-S`) inserts a bare run inside the other author's wrapper; a span crossing out of the wrapper takes `rebuildAcrossBoundaries`, whose two halves still share the original `w:id`; a `--current` match inside another author's `` still lands the replacement inside that deletion. The `track-changes list` text table pairs an adjacent del+ins into a `revN` without checking authors, so a split like this reads as `rev0 replace "cut" → ""`; `--json` carries each revision's author. +`replaceSpanInParagraph` (the engine behind `replace --track` and `edit --at pN:S-E --track`) has ONE early branch, dispatching to [core/find/replace-in-revision.tsx](../find/replace-in-revision.tsx). **The gate**: the replace is tracked, the span is non-empty, every overlapping run shares the same immediate parent, and that parent is a `` / `` sitting DIRECTLY under the paragraph. Everything else — untracked, empty spans, a span crossing the wrapper's edge, a wrapper nested in a hyperlink/smartTag/field, a match inside a `` under `--current`/`--baseline` — stays on the general walkers, byte-for-byte as before. Inside the gate, `w:author` decides (exact string match): **the wrapper is split the way Word does it** — it keeps its `w:id` plus the text before the span and the editor's `` of the cut nested inside it (a del-of-ins, so rejecting either author's revision restores the right text), the replacement rides the editor's own paragraph-level ``, and the content after the span rides a copy of the wrapper with a FRESH id from the shared allocator — unless that tail is pure markers (comment range end + reference run, bookmark end), which stay in the wrapper after the cut instead of minting an empty revision (`isPureMarker`). **The editor's own `` is rebuilt in place** (same id, same date, no ``, no split) — the replacement is just more of that same insertion — but every non-text child of the cut runs (note/comment reference, drawing, text-box anchor) is kept (`cutRemnants`), so nothing anchored on the replaced words vanishes. A `` is split even by its own author: moved text is relocated original text, not new words. `revN` pairing in `cli/track-changes/groups.ts` requires both halves to share an author, so the other author's head ins never pairs with the editor's nested del. The new path computes the wrapper's start offset from the siblings before it, where the general `rebuildContainer` starts its walk at the first OVERLAPPING run's offset — correct only when the match sits in the wrapper's first run, which is the "deleted the wrong words" half of the bug (the nested `` in the issue is just what put the match in a later run; the same miscount still applies to an untracked multi-run wrapper and to hyperlinks, both deliberately untouched here). Nested non-run children (a third author's ``, bookmarks) move WHOLE, counted at their view-visible length (a `` is 0 wide in the accepted view), and are never wrapped in the new ``: one inside the span keeps its place, so the cut is deleted as one `` per contiguous run group. Known limitations, all deliberately left on the old paths: an insertion nested in a hyperlink still gets a bare run inside it; an empty span (`pN:S-S`) inserts a bare run inside the other author's wrapper; a span crossing out of the wrapper takes `rebuildAcrossBoundaries`, whose two halves still share the original `w:id`; a `--current` match inside another author's `` still lands the replacement inside that deletion. The `track-changes list` text table pairs an adjacent del+ins into a `revN` without checking authors, so a split like this reads as `rev0 replace "cut" → ""`; `--json` carries each revision's author. ## Adding a new tracked-change kind diff --git a/tests/cli/replace.test.ts b/tests/cli/replace.test.ts index 119253f..c0e3450 100644 --- a/tests/cli/replace.test.ts +++ b/tests/cli/replace.test.ts @@ -1,7 +1,12 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { join } from "node:path"; +import { Pkg } from "@core/ast/document/package"; import { runCli, tempWorkspace } from "./harness"; -import { buildRawDoc, readDocumentXml } from "./helpers"; +import { + buildRawDoc, + readDocumentXml, + wordTextBoxParagraphXml, +} from "./helpers"; type Body = { blocks: Array<{ @@ -1255,6 +1260,10 @@ describe("docx replace --track inside another author's insertion (#13)", () => { ); } + async function readPart(docPath: string, name: string): Promise { + return await (await Pkg.open(docPath)).readText(name); + } + async function viewText(docPath: string, view: string): Promise { const result = await runCli("read", docPath, view); return result.stdout @@ -1300,6 +1309,18 @@ describe("docx replace --track inside another author's insertion (#13)", () => { ["ins", "Editor", "disputed sum"], ["ins", "Reviewer A", "."], ]); + // revN pairing is per author: the editor's del+ins is one group, and + // Reviewer A's head ins never pairs with the editor's nested del. + const grouped = changes as Array<{ author: string; group?: string }>; + const editorGroups = grouped + .filter((change) => change.author === "Editor") + .map((change) => change.group); + expect(editorGroups).toEqual(["rev0", "rev0"]); + expect( + grouped + .filter((change) => change.author === "Reviewer A") + .every((change) => change.group === undefined), + ).toBe(true); }); test("case 2: a nested deletion by a third author stays put and does not shift the cut", async () => { @@ -1435,6 +1456,190 @@ describe("docx replace --track inside another author's insertion (#13)", () => { ); }); + test("the author's OWN ins keeps a footnote reference and a text-box anchor that sat inside the cut", async () => { + const docPath = join(tempWorkspace("ins-own-anchors"), "out.docx"); + await runCli("create", docPath, "--text", "seed"); + expect( + ( + await runCli( + "footnotes", + "add", + docPath, + "--at", + "p0", + "--text", + "the note", + ) + ).exitCode, + ).toBe(0); + const footnotesBefore = await readPart(docPath, "word/footnotes.xml"); + // Word's text-box run, anchored mid-word ("am|ount"), and the footnote + // reference run the CLI emits — both inside Reviewer A's insertion. + const box = wordTextBoxParagraphXml(["box story"], { leadingText: "am" }); + const namespaces = box.slice("")); + const shapeRun = box.slice( + box.indexOf(""), + box.lastIndexOf("") + "".length, + ); + const paragraph = + `The Client ${INS_A}` + + 'may withhold a disputed' + + '' + + ` ${shapeRun}ount.`; + expect( + ( + await runCli( + "raw", + "replace", + docPath, + "--at", + "p0", + "--xml", + paragraph, + ) + ).exitCode, + ).toBe(0); + const before = await runCli("read", docPath); + expect(before.stdout).toContain( + "The Client may withhold a disputed[^fn1] amount.", + ); + expect(before.stdout).toContain("box story"); + + const result = await runCli( + "replace", + docPath, + "disputed amount", + "disputed sum", + "--track", + "--author", + "Reviewer A", + ); + expect(result.exitCode).toBe(0); + const after = await runCli("read", docPath); + expect(after.stdout).toContain( + "The Client may withhold a disputed sum[^fn1].", + ); + expect(after.stdout).toContain("docx:textbox tbx0"); + expect(after.stdout).toContain("box story"); + const xml = await readDocumentXml(docPath); + expect(xml).not.toContain("/g)).toHaveLength(2); + expect(await readPart(docPath, "word/footnotes.xml")).toBe(footnotesBefore); + }); + + test("the author's OWN moveTo is split, never merged into (moved text is not new words)", async () => { + const MOVED = + 'Origin: the moved sentence' + + 'Destination: the moved sentence.'; + const docPath = await buildRawDoc(MOVED, "moveto-own"); + const result = await runCli( + "replace", + docPath, + "moved", + "relocated", + "--track", + "--author", + "Reviewer", + ); + expect(result.exitCode).toBe(0); + expect((await runCli("validate", docPath)).exitCode).toBe(0); + + const xml = (await readDocumentXml(docPath)).replaceAll( + / w:date="[^"]*"/g, + "", + ); + const destination = xml.match( + /(?:(?!).)*Destination.*?<\/w:p>/s, + )?.[0]; + expect(destination).toBe( + 'Destination: ' + + 'the ' + + 'moved' + + 'relocated' + + ' sentence' + + '.', + ); + expect(await viewText(docPath, "--accepted")).toBe( + "Origin:\nDestination: the relocated sentence.", + ); + expect((await listChanges(docPath)).map((change) => change.kind)).toEqual([ + "moveFrom", + "moveTo", + "del", + "ins", + "moveTo", + ]); + }); + + test("a comment anchored on the replaced phrase keeps its markers; a marker-only tail is not a revision", async () => { + const docPath = await buildRawDoc( + `The Client ${INS_A}may withhold a disputed amount`, + "ins-comment-tail", + ); + expect( + ( + await runCli( + "comments", + "add", + docPath, + "--anchor", + "disputed amount", + "--text", + "why?", + ) + ).exitCode, + ).toBe(0); + const result = await runCli( + "replace", + docPath, + "disputed amount", + "disputed sum", + "--track", + "--author", + "Editor", + ); + expect(result.exitCode).toBe(0); + expect((await runCli("validate", docPath)).exitCode).toBe(0); + + // The tail (comment range end + reference run) is pure markers: it stays + // in Reviewer A's ins after the cut — no fresh around zero + // content, so no empty revision, and the comment still brackets the text. + expect(await paragraphXml(docPath, "Editor")).toBe( + 'The Client ' + + `${INS_A}may withhold a ` + + 'disputed amount' + + '' + + 'disputed sum', + ); + const changes = await listChanges(docPath); + expect(changes.map((change) => [change.kind, change.author])).toEqual([ + ["ins", "Reviewer A"], + ["del", "Editor"], + ["ins", "Editor"], + ]); + expect(changes.every((change) => change.text.length > 0)).toBe(true); + const list = await runCli("comments", "list", docPath); + const comments = list.parsed as Array<{ + id: string; + anchor: { startBlockId: string }; + }>; + expect(comments.map((comment) => [comment.id, comment.anchor])).toEqual([ + ["c0", expect.objectContaining({ startBlockId: "p0" })], + ]); + + // Rejecting everything removes A's insertion with the comment markers it + // held — nothing dangles. + const rejected = await runCli("track-changes", "reject", docPath, "--all"); + expect(rejected.exitCode).toBe(0); + const after = await readDocumentXml(docPath); + expect(after).not.toContain(" { // Only a match wholly inside the wrapper is gated; this one starts in the // plain run before it. The expected string is main's output, pinned From 1974ee7879a7faee184f710f2aef1109493246a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 22:04:49 +0000 Subject: [PATCH 3/3] fix(replace): own-ins merge drops counted characters, brackets ranges; page-break tail is a marker (#13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final pass on the in-revision span replace: - `cutRemnants` keeps only ZERO-width children of the cut runs (note/comment references, drawings, text-box anchors — `inlineMarkerWidth === 0`, now exported from the parser barrel); a tab/break/sym the offsets counted as a character goes with the matched text instead of surviving the replacement. - Range-start markers, and anything ahead of the first cut run, are emitted BEFORE the replacement; range ends, reference runs and kept remnants AFTER it — a comment that opened inside the cut brackets the new words instead of collapsing to a point. - `isPureMarker` treats a run of nothing but `w:rPr` / `w:commentReference` / `w:lastRenderedPageBreak` as a marker, so such a tail no longer mints a phantom fresh-id ``. - Docs: drop the stale "pairs without checking authors" sentence; state the author-equality rule in the revN section of cli/track-changes/CLAUDE.md. Triage-Routine: true Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012GoxD36rnNfg6uArkVxtJL --- src/cli/track-changes/CLAUDE.md | 2 +- src/core/find/replace-in-revision.tsx | 72 +++++++++++++++++------ src/core/parser/index.ts | 1 + src/core/parser/run-ops.ts | 2 +- src/core/track-changes/CLAUDE.md | 2 +- tests/cli/replace.test.ts | 83 +++++++++++++++++++++++++++ 6 files changed, 140 insertions(+), 22 deletions(-) diff --git a/src/cli/track-changes/CLAUDE.md b/src/cli/track-changes/CLAUDE.md index c6693ec..2c52242 100644 --- a/src/cli/track-changes/CLAUDE.md +++ b/src/cli/track-changes/CLAUDE.md @@ -41,7 +41,7 @@ Still out of scope: `` (RUN-property revisions) and `` + an adjacent ``). Accepting them one at a time forces a re-`list` between each, because `tcN` ids renumber after every accept — the `contract-finalize` friction. [`groups.ts`](groups.ts) (`revisionGroups`) pairs an adjacent del+ins / ins+del **on the same paragraph** into one `revN` handle: `list` tags both halves with a shared `"group": "revN"`, and the CLI verbs expand `--at revN` (or `--accept/--reject revN`) to the member `tcN`s (`expandRevisionTargets`) **before** handing them to the core accept/reject engine. So `accept --at revN` is one call for one logical change. -This is a **pure CLI-layer derived view** — it never touches the reader, the `tcN` ids, or the core state machine. `revisionGroups` is computed identically (sort by tcN index, then pair) in `list-view.ts` (the `list` table), `run-apply.ts` (`accept`/`reject`), and `apply.ts` (`apply`), so they can't disagree. Grouping is conservative: only plain `ins`/`del` pair (not moveFrom/moveTo, paragraph-mark, table, or property revisions), so `ins+ins` (a paragraph insert's run + paragraph-mark) and `del+del` never group. Solo changes get no `group`. `revN` is addressing sugar only — `tcN` stays the granular handle. +This is a **pure CLI-layer derived view** — it never touches the reader, the `tcN` ids, or the core state machine. `revisionGroups` is computed identically (sort by tcN index, then pair) in `list-view.ts` (the `list` table), `run-apply.ts` (`accept`/`reject`), and `apply.ts` (`apply`), so they can't disagree. Grouping is conservative: only plain `ins`/`del` pair (not moveFrom/moveTo, paragraph-mark, table, or property revisions), so `ins+ins` (a paragraph insert's run + paragraph-mark) and `del+del` never group — and **both halves must share an author**: one author's `` nested in another's `` (the cross-author span replace of issue #13, see [core/track-changes/CLAUDE.md](../../core/track-changes/CLAUDE.md)) is two decisions, not one replace. Solo changes get no `group`. `revN` is addressing sugar only — `tcN` stays the granular handle. ## tcN ids: one walk, one source of truth diff --git a/src/core/find/replace-in-revision.tsx b/src/core/find/replace-in-revision.tsx index c9bb63e..afeb75e 100644 --- a/src/core/find/replace-in-revision.tsx +++ b/src/core/find/replace-in-revision.tsx @@ -1,4 +1,10 @@ -import { runTextLength, sliceRun, wrapperContent, XmlNode } from "../parser"; +import { + inlineMarkerWidth, + runTextLength, + sliceRun, + wrapperContent, + XmlNode, +} from "../parser"; import { convertTextToDelText, type TrackedMeta } from "../track-changes"; import { Del, Ins } from "../track-changes/emit"; import type { FindView } from "./index"; @@ -28,8 +34,10 @@ import { * the wrapper after the cut rather than becoming an empty revision. The one * exception to splitting is the editor's OWN ``, rebuilt in place * (same id, same date): the replacement is just more of that same insertion, - * so no split and no `` — though every non-text child of the cut (a - * note or comment reference, a drawing, a text-box anchor) is kept. A + * so no split and no `` — the cut's characters go, but every + * zero-width child (a note or comment reference, a drawing, a text-box + * anchor) is kept, range starts ahead of the replacement and the rest behind + * it, so a range that opened inside the cut brackets the new words. A * `` is never merged into, even by its own author: moved text is * relocated original text, not the author's new words. * @@ -60,10 +68,12 @@ export function replaceSpanInsideRevision( wrapper.tag === "w:ins" && wrapper.attributes["w:author"] === tracked.meta.author ) { + const remnants = cutRemnants(parts.cut); wrapper.children = [ ...parts.pre, + ...remnants.before, ...runs, - ...cutRemnants(parts.cut), + ...remnants.after, ...parts.post, ]; return; @@ -132,26 +142,45 @@ function partitionWrapperChildren( return { pre, cut, post }; } -/** What the editor's own cut leaves behind, in order: every non-run child, - * and each cut run stripped of its text — a run that still carries a child - * beyond `` (a note or comment reference, a drawing, a text-box - * anchor) stays, so whatever was anchored on the replaced words survives; - * a text-only run is dropped. */ -function cutRemnants(cut: XmlNode[]): XmlNode[] { - const out: XmlNode[] = []; +/** What the editor's own cut leaves behind, split around the replacement. + * Every character of the cut goes — ``/`` and the width-1 + * equivalents (``, ``, ``, …) the offsets counted — but + * each zero-width child (a note or comment reference, a drawing, a text-box + * anchor, a page-break hint) is kept, so whatever was anchored on the + * replaced words survives. Range starts, and anything ahead of the first cut + * run, go BEFORE the replacement; range ends, reference runs and the kept + * remnants go AFTER it — a range that opened inside the cut brackets the new + * words instead of collapsing to a point. */ +function cutRemnants(cut: XmlNode[]): { before: XmlNode[]; after: XmlNode[] } { + const before: XmlNode[] = []; + const after: XmlNode[] = []; + let seenRun = false; for (const child of cut) { if (child.tag !== "w:r") { - out.push(child); + if (!seenRun || RANGE_START_TAGS.has(child.tag)) before.push(child); + else after.push(child); continue; } + seenRun = true; child.children = child.children.filter( - (part) => part.tag !== "w:t" && part.tag !== "w:delText", + (part) => + part.tag !== "w:t" && + part.tag !== "w:delText" && + inlineMarkerWidth(part) === 0, ); - if (child.children.some((part) => part.tag !== "w:rPr")) out.push(child); + if (child.children.some((part) => part.tag !== "w:rPr")) after.push(child); } - return out; + return { before, after }; } +const RANGE_START_TAGS: ReadonlySet = new Set([ + "w:bookmarkStart", + "w:commentRangeStart", + "w:permStart", + "w:moveFromRangeStart", + "w:moveToRangeStart", +]); + /** The cut, deleted in document order: each contiguous group of runs becomes * one editor `` (text → delText); a non-run child between groups * passes through untouched, where it was. */ @@ -188,14 +217,19 @@ const PURE_MARKER_TAGS: ReadonlySet = new Set([ "w:proofErr", ]); +const PURE_MARKER_RUN_CHILDREN: ReadonlySet = new Set([ + "w:rPr", + "w:commentReference", + "w:lastRenderedPageBreak", +]); + /** A range/annotation marker that is not revision content: one of the - * paragraph-level marker tags, or a run holding only a comment reference. */ + * paragraph-level marker tags, or a run holding nothing but a comment + * reference and/or a rendered-page-break hint. */ function isPureMarker(node: XmlNode): boolean { if (PURE_MARKER_TAGS.has(node.tag)) return true; if (node.tag !== "w:r") return false; - return node.children.every( - (part) => part.tag === "w:rPr" || part.tag === "w:commentReference", - ); + return node.children.every((part) => PURE_MARKER_RUN_CHILDREN.has(part.tag)); } /** The tail of a split wrapper: same tag, author and date, fresh `w:id` diff --git a/src/core/parser/index.ts b/src/core/parser/index.ts index 8c72320..b10acb8 100644 --- a/src/core/parser/index.ts +++ b/src/core/parser/index.ts @@ -1,4 +1,5 @@ export { + inlineMarkerWidth, isRunBearingWrapper, isSubtractiveTrackedChangeWrapper, partitionParagraphRuns, diff --git a/src/core/parser/run-ops.ts b/src/core/parser/run-ops.ts index 17f6477..9f47450 100644 --- a/src/core/parser/run-ops.ts +++ b/src/core/parser/run-ops.ts @@ -123,7 +123,7 @@ export function sliceRun(run: XmlNode, start: number, end: number): XmlNode { * `core/ast/read.ts` and `paragraphTextForView` in `core/find/index.ts` — the * three must agree or `find` and `replace`/`comments add`/`hyperlinks add` * misalign. */ -function inlineMarkerWidth(child: XmlNode): number { +export function inlineMarkerWidth(child: XmlNode): number { switch (child.tag) { case "w:noBreakHyphen": case "w:softHyphen": diff --git a/src/core/track-changes/CLAUDE.md b/src/core/track-changes/CLAUDE.md index 1aebcb1..1daa0eb 100644 --- a/src/core/track-changes/CLAUDE.md +++ b/src/core/track-changes/CLAUDE.md @@ -83,7 +83,7 @@ rPr equality is structural (XML-string compare on cloned nodes). The codebase's ## Tracked span replace inside another author's insertion (issue #13) -`replaceSpanInParagraph` (the engine behind `replace --track` and `edit --at pN:S-E --track`) has ONE early branch, dispatching to [core/find/replace-in-revision.tsx](../find/replace-in-revision.tsx). **The gate**: the replace is tracked, the span is non-empty, every overlapping run shares the same immediate parent, and that parent is a `` / `` sitting DIRECTLY under the paragraph. Everything else — untracked, empty spans, a span crossing the wrapper's edge, a wrapper nested in a hyperlink/smartTag/field, a match inside a `` under `--current`/`--baseline` — stays on the general walkers, byte-for-byte as before. Inside the gate, `w:author` decides (exact string match): **the wrapper is split the way Word does it** — it keeps its `w:id` plus the text before the span and the editor's `` of the cut nested inside it (a del-of-ins, so rejecting either author's revision restores the right text), the replacement rides the editor's own paragraph-level ``, and the content after the span rides a copy of the wrapper with a FRESH id from the shared allocator — unless that tail is pure markers (comment range end + reference run, bookmark end), which stay in the wrapper after the cut instead of minting an empty revision (`isPureMarker`). **The editor's own `` is rebuilt in place** (same id, same date, no ``, no split) — the replacement is just more of that same insertion — but every non-text child of the cut runs (note/comment reference, drawing, text-box anchor) is kept (`cutRemnants`), so nothing anchored on the replaced words vanishes. A `` is split even by its own author: moved text is relocated original text, not new words. `revN` pairing in `cli/track-changes/groups.ts` requires both halves to share an author, so the other author's head ins never pairs with the editor's nested del. The new path computes the wrapper's start offset from the siblings before it, where the general `rebuildContainer` starts its walk at the first OVERLAPPING run's offset — correct only when the match sits in the wrapper's first run, which is the "deleted the wrong words" half of the bug (the nested `` in the issue is just what put the match in a later run; the same miscount still applies to an untracked multi-run wrapper and to hyperlinks, both deliberately untouched here). Nested non-run children (a third author's ``, bookmarks) move WHOLE, counted at their view-visible length (a `` is 0 wide in the accepted view), and are never wrapped in the new ``: one inside the span keeps its place, so the cut is deleted as one `` per contiguous run group. Known limitations, all deliberately left on the old paths: an insertion nested in a hyperlink still gets a bare run inside it; an empty span (`pN:S-S`) inserts a bare run inside the other author's wrapper; a span crossing out of the wrapper takes `rebuildAcrossBoundaries`, whose two halves still share the original `w:id`; a `--current` match inside another author's `` still lands the replacement inside that deletion. The `track-changes list` text table pairs an adjacent del+ins into a `revN` without checking authors, so a split like this reads as `rev0 replace "cut" → ""`; `--json` carries each revision's author. +`replaceSpanInParagraph` (the engine behind `replace --track` and `edit --at pN:S-E --track`) has ONE early branch, dispatching to [core/find/replace-in-revision.tsx](../find/replace-in-revision.tsx). **The gate**: the replace is tracked, the span is non-empty, every overlapping run shares the same immediate parent, and that parent is a `` / `` sitting DIRECTLY under the paragraph. Everything else — untracked, empty spans, a span crossing the wrapper's edge, a wrapper nested in a hyperlink/smartTag/field, a match inside a `` under `--current`/`--baseline` — stays on the general walkers, byte-for-byte as before. Inside the gate, `w:author` decides (exact string match): **the wrapper is split the way Word does it** — it keeps its `w:id` plus the text before the span and the editor's `` of the cut nested inside it (a del-of-ins, so rejecting either author's revision restores the right text), the replacement rides the editor's own paragraph-level ``, and the content after the span rides a copy of the wrapper with a FRESH id from the shared allocator — unless that tail is pure markers (comment range end + reference run, bookmark end), which stay in the wrapper after the cut instead of minting an empty revision (`isPureMarker`). **The editor's own `` is rebuilt in place** (same id, same date, no ``, no split) — the replacement is just more of that same insertion — but every ZERO-width child of the cut runs (note/comment reference, drawing, text-box anchor — `inlineMarkerWidth === 0`; a tab/break/sym the offsets counted as a character goes with the text) is kept (`cutRemnants`), range starts ahead of the replacement and the rest behind it, so nothing anchored on the replaced words vanishes and a comment that opened inside the cut brackets the new words. A `` is split even by its own author: moved text is relocated original text, not new words. `revN` pairing in `cli/track-changes/groups.ts` requires both halves to share an author, so the other author's head ins never pairs with the editor's nested del. The new path computes the wrapper's start offset from the siblings before it, where the general `rebuildContainer` starts its walk at the first OVERLAPPING run's offset — correct only when the match sits in the wrapper's first run, which is the "deleted the wrong words" half of the bug (the nested `` in the issue is just what put the match in a later run; the same miscount still applies to an untracked multi-run wrapper and to hyperlinks, both deliberately untouched here). Nested non-run children (a third author's ``, bookmarks) move WHOLE, counted at their view-visible length (a `` is 0 wide in the accepted view), and are never wrapped in the new ``: one inside the span keeps its place, so the cut is deleted as one `` per contiguous run group. Known limitations, all deliberately left on the old paths: an insertion nested in a hyperlink still gets a bare run inside it; an empty span (`pN:S-S`) inserts a bare run inside the other author's wrapper; a span crossing out of the wrapper takes `rebuildAcrossBoundaries`, whose two halves still share the original `w:id`; a `--current` match inside another author's `` still lands the replacement inside that deletion. ## Adding a new tracked-change kind diff --git a/tests/cli/replace.test.ts b/tests/cli/replace.test.ts index c0e3450..03cf125 100644 --- a/tests/cli/replace.test.ts +++ b/tests/cli/replace.test.ts @@ -1640,6 +1640,89 @@ describe("docx replace --track inside another author's insertion (#13)", () => { expect(await viewText(docPath, "--accepted")).toBe("The Client"); }); + test("the author's OWN ins drops a tab that was part of the match (only zero-width children survive)", async () => { + const docPath = await buildRawDoc( + `${INS_A}Total:100 due`, + "ins-own-tab", + ); + // The agent types `\t`; the CLI ingress decodes it to a real tab. + const result = await runCli( + "replace", + docPath, + "Total:\\t100", + "Sum 200", + "--track", + "--author", + "Reviewer A", + ); + expect(result.exitCode).toBe(0); + expect(await viewText(docPath, "--accepted")).toBe("Sum 200 due"); + expect(await paragraphXml(docPath, "Editor")).toBe( + `${INS_A}Sum 200 due`, + ); + }); + + test("a comment that opened inside the cut brackets the replacement in the author's OWN ins", async () => { + const docPath = await buildRawDoc( + `${INS_A}pay the disputed amount now.`, + "ins-own-comment", + ); + expect( + ( + await runCli( + "comments", + "add", + docPath, + "--anchor", + "amount", + "--text", + "which?", + ) + ).exitCode, + ).toBe(0); + const result = await runCli( + "replace", + docPath, + "disputed amount", + "disputed sum", + "--track", + "--author", + "Reviewer A", + ); + expect(result.exitCode).toBe(0); + expect(await paragraphXml(docPath, "Editor")).toContain( + 'disputed sum', + ); + const comments = (await runCli("comments", "list", docPath)) + .parsed as Array<{ + anchor: { startOffset: number; endOffset: number }; + }>; + expect(comments.map((comment) => comment.anchor)).toEqual([ + expect.objectContaining({ startOffset: 8, endOffset: 20 }), + ]); + const rendered = await runCli("read", docPath, "--comments"); + expect(rendered.stdout).toContain("pay the disputed sum[^c0] now."); + expect(rendered.stdout).toContain('[^c0]: "disputed sum"'); + }); + + test("a tail holding only a rendered-page-break run is not a revision", async () => { + const docPath = await trackedReplace( + `The Client ${INS_A}may withhold a disputed amount`, + "ins-pagebreak-tail", + "Editor", + ); + expect(await paragraphXml(docPath, "Editor")).toBe( + 'The Client ' + + `${INS_A}may withhold a ` + + 'disputed amount' + + "" + + 'disputed sum', + ); + expect((await listChanges(docPath)).map((change) => change.author)).toEqual( + ["Reviewer A", "Editor", "Editor"], + ); + }); + test("a span CROSSING out of the ins keeps the general walker's shape", async () => { // Only a match wholly inside the wrapper is gated; this one starts in the // plain run before it. The expected string is main's output, pinned