Skip to content

fix(track-changes): show tracked equations, images and line breaks - #27

Open
xsvm wants to merge 3 commits into
kklimuk:mainfrom
xsvm:fix/equation-revisions
Open

xsvm wants to merge 3 commits into
kklimuk:mainfrom
xsvm:fix/equation-revisions

Conversation

@xsvm

@xsvm xsvm commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Summary

A tracked-deleted equation was invisible to an agent. I hit this when an agent
marked equations in a report as deleted and then couldn't confirm the result
with read or track-changes list. Reproducing it, I found three problems,
plus the same gap for other non-text runs:

Before Cause
Word's deleted E=mc^2 read as $${}^{}$$ Word puts the marker inside each math run (<m:r><w:del><m:t>E</m:t></w:del></m:r>, captured from Word 365), and handleRun only looked for a direct m:t
No deletion shown in any view, for Word's shape or ours (<w:del><m:oMath>) the reader never set trackedChange on an EquationRun
list: Word's shape missing entirely; ours labelled "¶ paragraph mark" inner markers were never registered, and list only took text from text runs
Same for a deleted image or line break: still shown in the accepted view, unmarked in --current, listed as "¶ paragraph mark" the view filter and list only looked at text runs

The fix reuses the existing paths instead of adding new ones

  • mathRunContent (equation/handlers.ts) reads a math run's m:rPr/m:t
    through that marker. Both handleRun and the run-format grouping use it.
  • One equation, one revision. registerMathRevisions groups an equation's
    markers into one tcN per kind and author (TrackedChangeReference.markers).
    It goes through registerTrackedChange, which is the existing wrapper
    registration pulled out of walkRunContainer, so ids still come from the one
    walk. When the group covers the whole equation it becomes
    EquationRun.trackedChange, exactly like an enclosing <w:del>.
  • apply expands a multi-marker change into its markers (markerParts) and
    runs the existing applyAccept/applyReject on each. No new actions:
    accept-del removes the marked runs and leaves the same empty skeleton Word
    leaves; reject unwraps them.
  • runRevision(run) (revision-visibility.ts) returns any run's revision.
    read's isRunVisible and list now use it for every tracked run type.
    markRevision/criticMarkup share the {--…--}[^tcN] wrapping that text
    segments already used. list describes non-text runs as $…$,
    [image img0], [line break] and [tab].
  • Schema: the trackedChange definition that was copied into TextRun and
    ImageRun is now one $defs.RunTrackedChange, also referenced from
    EquationRun, BreakRun and TabRun, which carried the field without
    declaring it. I also added EquationRun.comments, which was missing from the
    schema.

After (Word-made file):

tc0  delete  p1  "$E=mc^2$"
tc1  delete  p2  "$a+b$"
{--$$E=mc^2$$--}[^tc0]
Inline {--$a+b$--}[^tc1]{-- --}[^tc2]here.

Found but left out

Checked against a Word-made survey document:

  • Header/footer revisions. Inserted header text is dropped from read
    and headers list, and isn't listed. This lives in the marginals reader,
    so it belongs in a separate PR.
  • w:rPrChange. Already documented as out of scope.

Test plan

  • bun run check
  • tests/cli/equations.test.ts: Word's per-run shape covering LaTeX,
    whole deletion (list / --current / accepted view), accept and reject
    of the group, and partial deletion. It also covers our own
    <w:del><m:oMath> shape reading as a deletion.
  • tests/cli/track-changes.test.ts: a deleted line break and a tracked
    images delete are hidden, marked and listed.
  • Full test:unit shows no new failures. The remaining two are the
    Windows-only ones on main: the compiled-binary upgrade test and the
    render probe when there's no soffice.
  • Real files saved by Word 365: read, list, accept-all and
    reject-all. Reject-all passes docx validate, and so does accept-all;
    Word's own file reports schema errors for its in-run <w:del>.

Word records a tracked equation edit inside the equation, one <w:del> per
math run, so a deleted E=mc^2 read as {}^{} and never listed. Read math runs
through that marker, register an equation's markers as one revision that
accept/reject apply per marker, and let every tracked run type follow the
view rule, CriticMarkup and list text that only text runs had.
@kklimuk

kklimuk commented Oct 28, 2026 •

Copy link
Copy Markdown
Owner

🤖 docx-cli triage

Thanks @xsvm, this is a solid fix. I reviewed head 772d526. bun run check passes and bun run test:unit passes (1916 tests).

What the PR claims

Tracked equations, images, line breaks and tabs were invisible to an agent: not shown in read, and listed as "¶ paragraph mark" or not at all. The PR makes them follow the same view rule as text and groups Word's per-math-run markers into one tcN.

Before / after (minimal hand-built docs, Word's <m:r><w:del>… shape)

doc origin/main this PR
whole E=mc^2 deleted, then an <w:ins> paragraph read: $${}^{}$$. list: only tc0 insert "added after" list: tc0 delete p1 "$E={mc}^2$", tc1 insert …. --current: {--$$E={mc}^2$$--}[^tc0]. Accepted view hides it
inline $a+b$ deleted no tracked changes tc0 delete p0 "$a+b$", Inline {--$a+b$--}[^tc0] here.
inserted equation no tracked changes tc0 insert "$a+b$", reject --at tc0 removes it
tracked <w:tab/> / <w:br/> in a table cell ¶ paragraph mark, unmarked "[tab]" / "[line break]", one{--<br>--}[^tc0]two, accepted view onetwo
in-equation replace (del x / ins y) invisible rev0 replace "$x$" → "$y$". Accept leaves $$y$$, reject leaves $$x$$

accept --all, reject --all and apply --accept tc0 --reject tc1 all work, and docx validate passes on every output.

Fixes I made (commit 12d3ba8, which I couldn't push; see below)

/code-review found three regressions against main plus some smaller gaps. Fixes, with tests:

  1. A partly revised equation showed its deleted symbols in the default (accepted) view. E<del>X</del>=1 read as $EX=1$, where main reads $E=1$. equations edit rebuilds from the cached LaTeX, so it then brought the deleted X back as live text. The fix computes the LaTeX for a partly revised equation from a copy with the w:del markers removed (withoutMathDeletions), so read matches accept --all.
  2. mathRunContent read only the first marker. It missed m:rPr outside the marker (\sin x read as sinx) and every marker after the first in one run (<w:del>2</w:del><w:ins>3</w:ins> showed only 2). It now opens every marker in the run.
  3. The schema shape, <w:del> wrapping whole math elements inside <m:oMath>, lost its structure (Ec2 instead of Ec^2). w:ins/w:del are now transparent LaTeX handlers.
  4. A comment marker on an equation went missing after a hidden run in the same paragraph: commentEndingsFor was keyed on the run's position among visible runs (cursor) instead of its position in the paragraph (entry.originalIndex). The PR hides more runs, so this happened more often.
  5. A tracked text-box anchor now lists as [text box tbxN], and the schema declares trackedChange on TextBoxRun.

After the fixes, check passes and unit tests pass (1918 tests).

I couldn't push: the sandbox's git proxy rejected the push to xsvm/docx-cli. The full patch is below. It applies on top of 772d526 with git am.

fix.patch
From 12d3ba88d97f9b32d4eed45ae8a22bca0360cb21 Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Wed, 28 Oct 2026 13:13:49 +0000
Subject: [PATCH] fix(track-changes): read partly revised equations as
 accepted, keep math structure

- A partly revised equation's LaTeX is built with its w:del markers removed,
  so the default read matches accept --all (was showing deleted symbols).
- mathRunContent opens every marker in a run, so rPr outside a marker and
  several markers in one run both read correctly.
- w:ins/w:del wrapping whole math elements is transparent to the LaTeX walker.
- Equation comment endings key on the run's original index, not its visible
  position (hidden runs before it dropped the [^cN] marker).
- Text-box anchors list as "[text box tbxN]"; schema declares TextBoxRun
  trackedChange.

Triage-Routine: true
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkctEKmxkKz4Bm7VCCQ8x3
---
 src/cli/info/schema.ts                        |  1 +
 src/cli/read/markdown.ts                      |  9 ++---
 src/cli/track-changes/list-view.ts            |  2 +
 src/core/ast/read.ts                          | 37 +++++++++++++++----
 src/core/equation/handlers.ts                 | 26 ++++++++-----
 src/core/equation/read.ts                     |  2 +-
 src/core/track-changes/revision-visibility.ts |  2 +-
 tests/cli/equations.test.ts                   | 16 ++++++++
 tests/cli/track-changes.test.ts               | 12 ++++++
 9 files changed, 82 insertions(+), 25 deletions(-)

diff --git a/src/cli/info/schema.ts b/src/cli/info/schema.ts
index 8cc172e..66403b7 100644
--- a/src/cli/info/schema.ts
+++ b/src/cli/info/schema.ts
@@ -273,6 +273,7 @@ const JSON_SCHEMA = {
 				floating: { type: "boolean" },
 				wrap: { type: "string" },
 				align: { type: "string" },
+				trackedChange: { $ref: "#/$defs/RunTrackedChange" },
 				blocks: { type: "array", items: { $ref: "#/$defs/Block" } },
 			},
 		},
diff --git a/src/cli/read/markdown.ts b/src/cli/read/markdown.ts
index 14f4656..3297845 100644
--- a/src/cli/read/markdown.ts
+++ b/src/cli/read/markdown.ts
@@ -1514,12 +1514,9 @@ function renderRuns(
 				ctx,
 			);
 			// Append any comment endings that close on this equation run
-			// (audit comments from tracked equation edits anchor here).
-			out += commentEndingsFor(
-				paragraphId,
-				[{ originalIndex: cursor }],
-				ctx.commentIndex,
-			);
+			// (audit comments from tracked equation edits anchor here). Slots are
+			// keyed by the run's index in the paragraph, not among visible runs.
+			out += commentEndingsFor(paragraphId, [entry], ctx.commentIndex);
 		} else if (run.type === "noteRef") {
 			if (run.kind === "footnote") ctx.referencedFootnoteIds.add(run.id);
 			else ctx.referencedEndnoteIds.add(run.id);
diff --git a/src/cli/track-changes/list-view.ts b/src/cli/track-changes/list-view.ts
index fb2bf4b..c2b9d8b 100644
--- a/src/cli/track-changes/list-view.ts
+++ b/src/cli/track-changes/list-view.ts
@@ -135,6 +135,8 @@ function recordText(run: Run): string {
 			return `[${run.kind} break]`;
 		case "tab":
 			return "[tab]";
+		case "textBox":
+			return `[text box ${run.id}]`;
 		default:
 			return "";
 	}
diff --git a/src/core/ast/read.ts b/src/core/ast/read.ts
index f2f40b8..1095078 100644
--- a/src/core/ast/read.ts
+++ b/src/core/ast/read.ts
@@ -598,7 +598,13 @@ function walkRunContainer(
 			if (text.length > 0) {
 				const display = child.tag === "m:oMathPara";
 				const id = `eq${context.state.equationIndex++}`;
-				const latex = ommlToLatex(child);
+				const revisions = registerMathRevisions(context, child, trackedChange);
+				// A partly revised equation has no run-level revision to hide it by,
+				// so its LaTeX is the accepted view `read` defaults to — what
+				// `accept --all` leaves — not the deleted symbols beside the kept ones.
+				const latex = ommlToLatex(
+					revisions.partial ? withoutMathDeletions(child) : child,
+				);
 				const run: EquationRun = {
 					type: "equation",
 					id,
@@ -609,8 +615,7 @@ function walkRunContainer(
 				if (context.activeComments.size > 0) {
 					run.comments = [...context.activeComments];
 				}
-				const change =
-					registerMathRevisions(context, child, trackedChange) ?? trackedChange;
+				const change = revisions.whole ?? trackedChange;
 				if (change) run.trackedChange = change;
 				context.paragraph.runs.push(run);
 				context.document.body.equationReferences.set(id, {
@@ -713,16 +718,18 @@ function registerTrackedChange(
  *  in every math run it touched and in each structure's `<m:ctrlPr>` — six
  *  markers for one deleted `E=mc^2`. Register them as ONE revision per kind and
  *  author, so the equation lists, accepts and rejects as a unit (apply runs the
- *  ordinary per-marker operation on each — see `ChangeFound.markers`). Returns
- *  the revision when it covers the whole equation, so it rides the run the way
- *  an enclosing `<w:del>` does. */
+ *  ordinary per-marker operation on each — see `ChangeFound.markers`). `whole`
+ *  is the revision when it covers the whole equation, so it rides the run the
+ *  way an enclosing `<w:del>` does; `partial` says the equation holds revisions
+ *  that don't. */
 function registerMathRevisions(
 	context: WalkContext,
 	equation: XmlNode,
 	outer: TrackedChange | undefined,
-): TrackedChange | undefined {
+): { whole?: TrackedChange; partial: boolean } {
 	const groups = new Map<string, TrackedChangeMarker[]>();
 	collectMathRevisionMarkers(equation, groups);
+	if (groups.size === 0) return { partial: false };
 	const coversEquation = groups.size === 1 && !hasUnmarkedMathText(equation);
 	let whole: TrackedChange | undefined;
 	for (const markers of groups.values()) {
@@ -737,7 +744,7 @@ function registerMathRevisions(
 		);
 		if (coversEquation) whole = change;
 	}
-	return whole;
+	return coversEquation ? { whole, partial: false } : { partial: true };
 }
 
 function collectMathRevisionMarkers(
@@ -765,6 +772,20 @@ function hasUnmarkedMathText(node: XmlNode): boolean {
 	});
 }
 
+/** A copy of `node` as it reads once its tracked deletions are accepted — a
+ *  copy, so the live tree keeps every marker for `accept`/`reject`. */
+function withoutMathDeletions(node: XmlNode): XmlNode {
+	const copy = new XmlNode(
+		node.tag,
+		node.attributes,
+		node.children
+			.filter((child) => child.tag !== "w:del")
+			.map(withoutMathDeletions),
+	);
+	if (node.text !== undefined) copy.text = node.text;
+	return copy;
+}
+
 const TRACKED_CHANGE_KIND_BY_TAG: Record<
 	"w:ins" | "w:del" | "w:moveFrom" | "w:moveTo",
 	TrackedChange["kind"]
diff --git a/src/core/equation/handlers.ts b/src/core/equation/handlers.ts
index 089383a..12b1c4f 100644
--- a/src/core/equation/handlers.ts
+++ b/src/core/equation/handlers.ts
@@ -15,6 +15,11 @@ import { convertChildren } from "./read";
 export const ELEMENT_HANDLERS: Record<string, (node: XmlNode) => string> = {
 	"m:r": handleRun,
 	"m:e": handleE,
+	// A revision marker wrapping whole math elements — the schema's shape for a
+	// tracked edit inside an equation (`<w:del><m:sSup>…</m:sSup></w:del>`) — is
+	// transparent like `<m:e>`: its content is the equation's.
+	"w:ins": handleE,
+	"w:del": handleE,
 	"m:sSup": handleSSup,
 	"m:sSub": handleSSub,
 	"m:sSubSup": handleSSubSup,
@@ -53,11 +58,14 @@ export function collectPlainText(node: XmlNode): string {
 	return out;
 }
 
-/** The element holding a math run's `m:rPr`/`m:t`. Word records a tracked edit
- *  of an equation INSIDE each run it touched — `<m:r><w:del>…<m:t>E</m:t></w:del></m:r>`
- *  — so the run's content sits one level down, in the revision marker. */
-export function mathRunContent(run: XmlNode): XmlNode {
-	return run.children.find((child) => MATH_REVISION_TAGS.has(child.tag)) ?? run;
+/** A math run's children with every revision marker in it opened up. Word
+ *  records a tracked edit of an equation INSIDE each run it touched —
+ *  `<m:r><w:del>…<m:t>E</m:t></w:del></m:r>` — so a run's `m:rPr`/`m:t` can sit
+ *  one level down, beside (or instead of) its own, in one marker or several. */
+export function mathRunContent(run: XmlNode): XmlNode[] {
+	return run.children.flatMap((child) =>
+		MATH_REVISION_TAGS.has(child.tag) ? child.children : [child],
+	);
 }
 
 /** Revision markers Word nests inside an equation (in `<m:r>` and `<m:ctrlPr>`). */
@@ -76,8 +84,8 @@ export const MATH_REVISION_TAGS = new Set(["w:ins", "w:del"]);
  *      the upright typesetting on re-write; bare `pH` would render italic)
  *    - everything else → escaped + Unicode-mapped LaTeX */
 function handleRun(run: XmlNode): string {
-	const node = mathRunContent(run);
-	const rPr = node.findChild("m:rPr");
+	const children = mathRunContent(run);
+	const rPr = children.find((child) => child.tag === "m:rPr");
 	const isPlain = rPr?.findChild("m:sty")?.getAttribute("m:val") === "p";
 	// A run with no formatting and just a literal space is our SpaceRun shape
 	// (writer emits this for `<mspace>` with non-zero width — `\,`, `\quad`,
@@ -85,7 +93,7 @@ function handleRun(run: XmlNode): string {
 	// pass (temml drops literal whitespace in math mode), so emit `\,` to
 	// re-trigger the same mspace.
 	if (!rPr) {
-		const t = node.findChild("m:t");
+		const t = children.find((child) => child.tag === "m:t");
 		const text = t?.collectText() ?? "";
 		// A run with no formatting and only literal spaces is our SpaceRun
 		// shape (writer emits 1 space for `\,`, 4 for `\quad`, 8 for `\qquad`,
@@ -97,7 +105,7 @@ function handleRun(run: XmlNode): string {
 		}
 	}
 	let out = "";
-	for (const child of node.children) {
+	for (const child of children) {
 		if (child.tag !== "m:t" && child.tag !== "m:delText") continue;
 		const raw = child.collectText();
 		if (isPlain && KNOWN_OPERATORS.has(raw)) {
diff --git a/src/core/equation/read.ts b/src/core/equation/read.ts
index b6eb8a8..595347e 100644
--- a/src/core/equation/read.ts
+++ b/src/core/equation/read.ts
@@ -105,7 +105,7 @@ type RunFormat = {
  *  (so the caller doesn't enter the grouping branch). */
 function extractRunFormat(node: XmlNode): RunFormat | undefined {
 	if (node.tag !== "m:r") return undefined;
-	const rPr = mathRunContent(node).findChild("m:rPr");
+	const rPr = mathRunContent(node).find((child) => child.tag === "m:rPr");
 	if (!rPr) return undefined;
 	const sty = rPr.findChild("m:sty")?.getAttribute("m:val");
 	const bold = sty === "b" || sty === "bi";
diff --git a/src/core/track-changes/revision-visibility.ts b/src/core/track-changes/revision-visibility.ts
index 8da2fa8..b8b51dc 100644
--- a/src/core/track-changes/revision-visibility.ts
+++ b/src/core/track-changes/revision-visibility.ts
@@ -25,7 +25,7 @@ export function isRevisionVisible(
 }
 
 /** The revision a run sits in, whatever its type — text, image, equation,
- *  line break and tab runs can all be tracked; the rest never are. */
+ *  line break, tab and text-box runs can all be tracked; the rest never are. */
 export function runRevision(run: Run): TrackedChange | undefined {
 	return "trackedChange" in run ? run.trackedChange : undefined;
 }
diff --git a/tests/cli/equations.test.ts b/tests/cli/equations.test.ts
index 18a09bc..52bec0e 100644
--- a/tests/cli/equations.test.ts
+++ b/tests/cli/equations.test.ts
@@ -938,11 +938,27 @@ describe("tracked equations — Word's per-run markers", () => {
 		// Not the whole equation, so the equation itself carries no deletion.
 		const current = await runCli("read", docPath, "--current");
 		expect(current.stdout).not.toContain("{--$$");
+		// The default read is what accept --all leaves — no deleted symbols.
+		expect((await runCli("read", docPath)).stdout).toContain("$$E={}^{}$$");
 		await runCli("track-changes", "accept", docPath, "--all");
 		const [equation] = await equationsOf(docPath);
 		// The emptied superscript stays behind as a skeleton, as it does in Word.
 		expect(equation?.latex).toBe("E={}^{}");
 	});
+
+	test("the schema's shape — a marker wrapping whole math elements — keeps its structure", async () => {
+		const mark = (inner: string, id: number): string =>
+			`<w:del w:id="${id}" w:author="Word User" w:date="2026-01-01T00:00:00Z">${inner}</w:del>`;
+		const docPath = await buildRawDoc(
+			`<w:p><m:oMath ${MATH_NS}>${mark("<m:r><m:t>E</m:t></m:r>", 1)}` +
+				`${mark("<m:sSup><m:e><m:r><m:t>c</m:t></m:r></m:e><m:sup><m:r><m:t>2</m:t></m:r></m:sup></m:sSup>", 2)}` +
+				"</m:oMath></w:p>",
+			"eq-schema-shape",
+		);
+		expect((await runCli("read", docPath, "--current")).stdout).toContain(
+			"{--$Ec^2$--}[^tc0]",
+		);
+	});
 });
 
 describe("tracked equations — our own wrapper around <m:oMath>", () => {
diff --git a/tests/cli/track-changes.test.ts b/tests/cli/track-changes.test.ts
index 317a9de..f0e4faa 100644
--- a/tests/cli/track-changes.test.ts
+++ b/tests/cli/track-changes.test.ts
@@ -2122,6 +2122,18 @@ describe("tracked non-text runs follow the same view rule as text", () => {
 		expect(list.map((entry) => entry.text)).toEqual(["[line break]"]);
 	});
 
+	test("a hidden run before a commented equation keeps the comment's marker", async () => {
+		const docPath = await buildRawDoc(
+			'<w:p><w:r><w:t>a</w:t></w:r><w:del w:id="1" w:author="Ed" w:date="2026-01-01T00:00:00Z"><w:r><w:br/></w:r></w:del>' +
+				'<w:commentRangeStart w:id="0"/><m:oMath xmlns:m="http://schemas.openxmlformats.org/officeDocument/2006/math"><m:r><m:t>y</m:t></m:r></m:oMath><w:commentRangeEnd w:id="0"/>' +
+				"<w:r><w:t>b</w:t></w:r></w:p>",
+			"tracked-break-comment",
+		);
+		expect((await runCli("read", docPath, "--comments")).stdout).toContain(
+			"a$y$[^c0]b",
+		);
+	});
+
 	test("a tracked image delete is hidden, marked, and listed as an image", async () => {
 		const docPath = await copyFixture(
 			"tracked-image-delete",
-- 
2.43.0

Docs

The two CLAUDE.md edits match the code. After the patch, src/core/equation/CLAUDE.md should also say that a partly revised equation's LaTeX is the accepted view and that mathRunContent now returns the run's children with every marker opened. I didn't edit those files (instruction files are review-only for the routine).

Open product questions

  • Is this change wanted? It's standalone, with no issue behind it. I'd say yes: it fixes an agent-visible blind spot.
  • Partial equation revisions get a tcN in list, but nothing in read --current / --ast points at it, and read --baseline can't show the pre-edit equation. Is that acceptable for now, or do you want per-view LaTeX or partial-revision markup in the AST? (This bears on the write-read-loop and "everything parsed is in the AST" invariants.)
  • An equation wholly deleted by two authors counts as partial: its accepted LaTeX is empty, so markdown falls back to the full deleted run.text.
  • Not done in this PR: nested markers inside a math marker (A's insertion holding B's deletion) aren't registered. Word's math markers in footnotes/endnotes (registerNoteRunChanges) still list as one tcN per marker. In --current, a tracked display equation keeps its locator on the same line ({--$$…$$--}[^tc0] <!-- p1 -->). Follow-ups, or should they block this PR?
  • The headers/footers revision gap the PR mentions belongs in a separate issue.

Action needed: routine blocked — apply fix.patch (above) to fix/equation-revisions, or ask @xsvm to, since the sandbox couldn't push to the fork. Then decide the questions above and merge. If you push the patch yourself, your approval alone won't satisfy the last-push rule: merge with admin bypass or get a second approver.


Generated by Claude Code

claude and others added 2 commits October 30, 2026 01:38
…ath structure

- A partly revised equation's LaTeX is built with its w:del markers removed,
  so the default read matches accept --all (was showing deleted symbols).
- mathRunContent opens every marker in a run, so rPr outside a marker and
  several markers in one run both read correctly.
- w:ins/w:del wrapping whole math elements is transparent to the LaTeX walker.
- Equation comment endings key on the run's original index, not its visible
  position (hidden runs before it dropped the [^cN] marker).
- Text-box anchors list as "[text box tbxN]"; schema declares TextBoxRun
  trackedChange.

Triage-Routine: true
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkctEKmxkKz4Bm7VCCQ8x3
@xsvm

xsvm commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. I applied your fix.patch unchanged as 7ba8ef0 (git am on top of 772d526), and added eb701db, which updates src/core/equation/CLAUDE.md as you suggested: partly revised equations read in the accepted view, mathRunContent opens every marker, and w:ins/w:del wrappers are transparent. I pushed both myself, so the last-push rule shouldn't get in the way.

Locally, biome, knip and tsc are clean, and equations.test.ts + track-changes.test.ts pass (143). Full CI is running on the PR.

On the open questions, I'm happy to do the follow-ups in separate PRs (nested markers, math markers in notes, partial-revision markup in the AST) if you want them. The header/footer revision gap is #28.

@kklimuk

kklimuk commented Oct 29, 2026

Copy link
Copy Markdown
Owner

🤖 docx-cli triage

Thanks @xsvm. I re-reviewed head eb701db.

What I verified

  • 7ba8ef0 is exactly the posted fix.patch. Its diff against 772d526 matches the patch line for line (same git patch-id), and it contains nothing else.
  • eb701db only touches src/core/equation/CLAUDE.md. The new text is accurate against the code: mathRunContent opens every marker, w:ins/w:del wrappers are transparent, and a partly revised equation reads in the accepted view.
  • bun run check passes. bun run test:unit passes (1918 tests). My first run had one failure that didn't come back when I re-ran it, and it didn't print details, so I'm treating it as a flaky test.

Before / after (hand-built docs using Word's <m:r><w:del>… shape)

doc origin/main (3c616a9) this PR
whole E=mc^2 deleted list: no tracked changes. read: $${}^{}$$ list: tc0 delete p1 "$E=mc^2$". The default read hides it. --current: {--$$E=mc^2$$--}[^tc0]
E<del>X</del>=1 list: no tracked changes list: tc0 delete p0 "$X$". read: $E=1$ (accepted view, no regression). Accept gives $E=1$, reject gives $EX=1$
deleted <w:br/> tc0 … ¶ paragraph mark, and the break still shows in read "[line break]". read: onetwo. --current marks it {--…--}[^tc0]

accept --all and reject --all work on all three, and docx validate passes on every output.

Changes

None from me this round. The PR is ready as it stands.

Open product questions (still open from the last review)

  • Do you want this change? Nobody filed an issue for it. My recommendation is yes, because it fixes something agents currently can't see.
  • A partial equation revision gets a tcN in list, but neither read --current nor --ast points at it. --ast shows latex: "E=1" next to text: "EX=1", and --baseline can't show the equation as it was before the edit. Is that OK for now? @xsvm has offered to add partial-revision markup to the AST in a follow-up.
  • An equation deleted entirely by two different authors counts as partial, so markdown falls back to the deleted run.text.
  • Should these be follow-ups or block this PR: nested markers inside a math marker, math markers in footnotes/endnotes (one tcN per marker), and the locator sitting on the same line as a tracked display equation in --current? @xsvm has offered separate PRs. Header/footer revisions are fix(headers): surface tracked changes inside header/footer parts #28.

Action needed: approve/merge — PR #27 is ready once you've answered the questions above. @xsvm made the last push, so your own approval satisfies the last-push rule.


Generated by Claude Code

@kklimuk

kklimuk commented Oct 30, 2026

Copy link
Copy Markdown
Owner

@xsvm hold off from making further changes till i've had a bit of time to read through and do my own reviews, it's gonna take me a few days.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants