Repository navigation
fix(docx-compare): apply a layered formatting revision per span of unchanged text, not per word - #1503
Conversation
…changed text, not per word A run that already carries a pending w:rPrChange and then gains bold while one of its words is replaced is compared word by word, so it reaches the layered-formatting splitter (#1464) as one run per word and space. The splitter replaced each of those runs with its own deleted/inserted copy pair, so a single sentence became 28 pairs and 56 rPrChange records (about 220 pairs and 438 duplicated prior records in one NVCA Stock Purchase Agreement paragraph). Accept All and Reject All were correct but the redline was unreadable. The splitter now joins each maximal sequence of adjacent sibling runs whose original and revised properties are identical into one deleted copy and one inserted copy holding the whole span; the replaced word between two spans stays the ordinary w:del/w:ins the comparison already emitted. The repro drops to three pairs. Runs holding distinct pending records (different IDs) are never joined, so each prior revision keeps its own record and PRESERVE semantics are unchanged. Fixes: #1488
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
LLM gate (advisory)All evaluated rules passed - 5 pass, 0 warn, 0 error, 11 skipped, 16 total FindingsNone. All 16 rules (5 evaluated, 11 skipped)
|
…copies and guard adjacent spans Review of #1503 found that the opt-in LibreOffice block asserted a positive result that LibreOffice 26.2 cannot give. Accept All and Reject All both keep the deleted and the inserted copy when each carries its own w:rPrChange. The same thing happens with the one-pair shape #1464 emits for bold alone and with the per-word pairs before this change. The block now pins that behaviour as a characterization, beside a plain deletion/insertion control that resolves correctly, so it fails when LibreOffice changes. Also add the review's stronger signature guard: directly adjacent bold and italic spans that share one pending record. A signature that ignored revised properties would join them, and this test fails on that mutation. Refs: #1488
…ng-coalesce-20261115
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Codex peer review, round 1: CHANGES_REQUIRED (reviewed
|
Codex peer review, round 2 (confirmation): APPROVE (reviewed
|
Post-merge smoke: PASSMerged as Before/after build: the PR build with the pre-fix (a) Original reproduction (
|
| case | grouping | this build w:del/w:ins/w:rPrChange |
before |
|---|---|---|---|
| word replaced + bold | readable-semantic | 3 / 3 / 6 | 28 / 28 / 56 |
| word replaced + bold | token-minimal | 3 / 3 / 6 | 28 / 28 / 56 |
| bold only | readable-semantic | 1 / 1 / 2 | 1 / 1 / 2 |
| bold only | token-minimal | 1 / 1 / 2 | 1 / 1 / 2 |
(b) Real documents: the layered paragraph in the dogfood "preserve-body" variant
The paragraph's run has a pending w:rPrChange, gains bold, and has one word replaced.
| document | this build | before |
|---|---|---|
| NVCA Stock Purchase Agreement | 3 / 3 / 6 | 219 / 219 / 438 |
| NVCA Voting Agreement | 3 / 3 / 6 | 112 / 112 / 224 |
| Common Paper CSA | 3 / 3 / 6 | 99 / 99 / 198 |
On all three, the schema gate (check_emitted_document_schema.mjs) passes. Accept All reproduces the revised document and Reject All reproduces the original, for every story part, with both docx-compare's acceptor and docx-core acceptChanges/rejectChanges. In the "full" variant, NVCA SPA and NVCA VA also give 3 / 3 / 6.
(c) Full dogfood harness (run-all.sh, compare phase)
| criterion | full: SPA / VA / CSA | boxes: SPA / VA / CSA | boxes + header pending: SPA / VA / CSA | preserve-body: SPA / VA / CSA |
|---|---|---|---|---|
| C1 compare succeeds | PASS / PASS / FAIL (#1502) | PASS ×3 | PASS ×3 | PASS ×3 |
| C2 unrepresented empty/expected | FAIL / FAIL (#1499) / – | PASS ×3 | PASS ×3 | PASS ×3 |
| C3 schema gate | PASS / PASS / – | PASS ×3 | PASS ×3 | PASS ×3 |
| C4 accept/reject faithful (all parts, both appliers) | PASS / PASS / – | FAIL / FAIL (#1489) / PASS | FAIL / FAIL (#1489) / PASS | PASS ×3 |
| C4b comparison-layer-only projections keep pending records | PASS / PASS / – | PASS ×3 | PASS ×3 | PASS ×3 |
| C5 revision IDs unique | PASS / PASS / – | PASS ×3 | PASS ×3 | PASS ×3 |
| C6 each change redlined in place | PASS / PASS / – | PASS ×3 | PASS ×3 | PASS ×3 |
| C7 LibreOffice | not run (see below) | not run | not run | not run |
| C8 Word | DEFERRED | DEFERRED | DEFERRED | DEFERRED |
None of the failures come from this PR:
- CSA full, C1: aborts with
word/header5.xml#w:txbxContent[0]: unpaired ancillary text-box story is not owned exclusively by a removed section slot. This is docx-compare: section-break text-box pairing aborts when a shared boxed footer changes in every section (regression from #1492) #1502, the regression from fix(docx-compare): pair header/footer text-box stories across an added or removed section break #1492. - Full, C2: header/footer entries are over-listed after the section-count change. This is docx-compare: header/footer entries stay listed after a section-count change when header/footer fields read other stories #1499.
- Boxes C4 on the NVCA documents: only the docx-core package applier (
DocxDocument.acceptChanges/rejectChanges) fails. It drops the emptyw:type="continuationNotice"footnote (w:id="1") from footnotes.xml. The comparison emits that note byte-identical to the revised input. The note survives when the inputs are resolved, because their footnotes carry no revisions. This is docx-core: acceptChanges/rejectChanges drop the continuationNotice footnote, leaving settings referencing it #1489, which predates this PR: the build without the fix emits byte-identicaldocument.xml,footnotes.xmlandendnotes.xmlfor all four boxes pairs.
LibreOffice: not run. On this host the LibreOffice oracle returned "produced no output for job0.odt" for all three variants: the macro fails to launch, as already noted in the PR's pre-submit. The PR body records the reader characterization from before.
Word: DEFERRED. The staged files are in ~/.cache/safe-docx-compare-fanout/word-staged/1488/.
Fixes #1488
Problem
A run that already carries a pending
w:rPrChange(PRESERVE: the original's revision stays and the comparison is layered on top) and then gains bold while one of its words is replaced exploded in the redline. The comparison splits that run word by word because a word changed, and the layered-formatting splitter from #1464 then replaced every word and space with its own deleted-copy/inserted-copy pair.iso-layered.mjs): 28w:del, 28w:ins, 56w:rPrChange.w:rPrChangerecords.Approach
splitLayeredRunFormatting(packages/docx-compare/src/tagged/layeredFormattingRevision.ts) now runs in two passes:w:rPrexactly, pending record included, and one inserted copy holding the revisedw:rPr. Each copy holds the span's whole content, with adjacentw:tjoined.The replaced word between two spans stays the ordinary
w:del/w:insthe comparison already emitted. Pending records keep theirw:idin the span signature, so runs holding distinct prior revisions are never merged and each prior revision keeps its own record. PRESERVE semantics do not change.Representation: run-level deleted and inserted content (ECMA-376 edition 5, Part 1 §17.13.5.14
w:del, §17.13.5.18w:ins), each copy carrying at most one run-property revision (§17.13.5.31w:rPrChange), as #1464 established. The repro becomes three pairs:The Company is not(layer),bound→REVISEDTERM(word),by any agreement …(layer). It previously had 28.The diff touches only
layeredFormattingRevision.tsand its test file. It does not touchpipeline.ts,textBoxRevisionSafety.tsor the serializer.Evidence
New tests in
packages/docx-compare/src/tagged/layeredFormattingRevision.test.ts, describe block "#1488":Repro shape under
token-minimalandreadable-semantic: at most 3w:del, at most 3w:insand at most 6w:rPrChange; the exact deleted/inserted texts; the schema gate (check_emitted_document_schema.mjs); one record per property element. Accept All matches the revised accepted view and Reject All matches the original rejected view, checked with both docx-compare's acceptor and docx-coreacceptChanges/rejectChanges. Selectively rejecting or accepting only the comparison's revisions gives back the original or revised document with pending records included.Distinct pending records (IDs 101 and 102) are never joined across their boundary. A mutation check that drops IDs from the signature makes this test fail.
Spans of different revised formatting (bold, then plain, then italic) stay separate pairs.
RED on base
e8d02e98: the two repro tests fail withexpected 28 to be less than or equal to 3, and the mixed-span test fails. All 21 existing docx-compare: a formatting change on a run that already has a pending rPrChange nests a second rPrChange (schema-invalid) #1461/fix(docx-compare): layer a formatting change onto a pending formatting revision without nesting #1464/docx-compare: adding or removing a section break on a paragraph with a pending pPrChange aborts #1465 tests still pass. A directly-adjacent bold/italic span guard (added after review) fails when the signature ignores revised properties.LibreOffice: characterization, not a positive result. LibreOffice 26.2.5.2 reads the deleted and inserted copies: an identity round trip keeps them. But Accept All and Reject All both leave the two copies in place whenever each copy carries its own
w:rPrChange. This is a limitation of the fix(docx-compare): layer a formatting change onto a pending formatting revision without nesting #1464 representation that was already there before this PR. It shows up the same way with:A plain
w:del/w:inscontrol in the same batch resolves correctly. The opt-in block (SAFE_DOCX_LAYERED_FORMATTING_READER_REQUIRED=1, run through the heavy-run gate, 1 passed) pins this behaviour beside the control, so it fails if LibreOffice changes. Microsoft Word, the behavioural oracle, is DEFERRED on this host. The LibreOffice gap needs its own follow-up.Real document (NVCA Stock Purchase Agreement, dogfood "preserve-body" recipe): the paragraph whose run carries a pending
w:rPrChange, gains bold and has one word replaced. Both builds compared the same original/revised pair:w:delw:insw:rPrChangee8d02e98On this PR's build, the dogfood harness reports PASS for compare, schema gate, accept/reject (all parts, both appliers), comparison-layer-only projections keeping pending records, unique revision IDs and every change redlined in place.
Pre-submit: build, lint:workspaces, test:run, check:spec-coverage, check:conformance-citations, check:conformance-doc and check:real-corpus (37/37) all pass, except two odf-core LibreOffice differential tests (
lo_paragraph_roundtrip,lo_convert_differential). Those fail on this host with "LibreOffice oracle produced no output". The host's LibreOffice macro fails to launch, and this diff does not touch odf-core, which does not depend on docx-compare.Left out
Nothing in this shape. Runs whose content cannot be repeated (field characters, drawings, note or comment references) or that sit where a run revision is not allowed still keep the single-record fallback from #1464; this PR does not change it.