Remove the WmlComparer engine in favour of DocxDiff - #643
Conversation
…s removed The four parity scoreboards check DocxDiff by running it head-to-head against WmlComparer over the WC corpus. They cannot outlive their own oracle, so this captures what they prove while both engines still exist. Two artifacts, with different jobs: docs/architecture/wmlcomparer_parity_baseline/ holds the four reports verbatim from their final run against a live WmlComparer. It is a historical record -- nothing can re-derive it once the legacy engine is gone. It answers "was the IR engine ever actually checked against the thing it replaced, and on what": 179 runnable cases at 177 pass / 2 documented deviations, 39 markup cases all pass, 84 Consolidate cases all reproduce, and 184 differential comparisons at 117 MATCH / 18 GRANULARITY / 47 catalogued DIVERGENT / 2 pairs the legacy engine threw on and DocxDiff handled. DocxDiffCorpusBaselineTests plus DocxDiffCorpusBaseline.tsv is the live half. It runs the same 92 pairs in both directions and both granularities -- 368 rows -- and pins DocxDiff's own per-kind multiset of normalized revision text. Pinning the multiset rather than a count means a regression that swaps which text is inserted, while keeping the tally, still fails. None of it touches WmlComparer: the differential harness only used the legacy engine for its expected side, and that side is now the committed file. Four whole-document-rewrite rows would otherwise be hundreds of KB each and dominate the file (the median row is under 100 bytes), so a row past 2000 chars collapses to per-kind tallies plus a SHA-256 over the same deterministic encoding -- still exact, just not readable. DOCXODUS_REGEN_CORPUS_BASELINE=1 rewrites the file and fails deliberately, so a regeneration can never be mistaken for a passing run. Verified: 368/368 rows reproduce.
BREAKING CHANGE: the WmlComparer comparison engine and its public surface are gone. DocxDiff has been the default since v8.0.0; this removes the alternative rather than keeping a frozen second engine alive behind a selector. Removed: WmlComparer (the engine, WmlComparerSettings, the nested revision types, ComparisonUnit*, CorrelationStatus, PtpSHA1Util, Base64Util), the ComparisonEngine selector enum, and ComparisonLog. The last three had no reachable purpose without the engine -- ComparisonLog was only ever populated through WmlComparerSettings.Log, so the two *WithLog surfaces had been returning an empty log on the DocxDiff path already. Kept, because it was never comparison logic: the cross-package merge helpers (CopyMissingStyles/CopyMissingNumbering/MoveRelatedPartsToDestination and their private helpers) move verbatim to Docxodus/PackageMerge.cs. DocxDiff's markup renderers have always called them. The move is byte-for-byte, #nullable disable header included, so a regression there can only come from a later edit. DocxCompare stays as the shared front door, minus the engine branch. This matters more than it looks: it always sets PreAcceptInputRevisions and PreserveInputRevisions, which the raw DocxDiff API leaves opt-in. That is the difference between Word-like behaviour and whole-document churn on revision-bearing inputs, and it was previously hidden inside a settings mapping that no longer has a reason to exist. It is now ApplyFrontDoorRevisionPolicy, applied to a clone so a caller's settings object is never mutated, and pinned by tests. Also dropped: the byte-identical-inputs shortcut used to refuse documents carrying tracked-revision markup inside an Office Math run, so they would route through the legacy path that repaired that schema-invalid shape. DocxDiff performs no such repair -- measured: same one validation error, byte-identical output -- so the guard only bought a full comparison that changed nothing. CanReturnExactNoOp is now plain byte equality, and a test pins the honest behaviour (invalid input survives rather than being silently rewritten). Repairing it is tracked as issue #642. Tests: the 73 WmlComparer-specific tests go. Tests that used the engine merely to synthesize a redline fixture now use DocxCompare. Two needed more care than a rename: - RP001 compared an accepted baseline against processed output by counting the revisions between them. Routed through the front door it saw the inputs' own preserved markup as a difference, and the engine additionally reports a paragraph-mark change as an empty-text revision, which the legacy reader dropped by its own normalize-and-drop-if-empty rule. It now asks the engine directly with pre-accept on, and applies that same empty-text rule. - The move-markup test asserted the reader saw two Moved revisions. The native reader groups a move into ONE entry and only emits it when both w:moveFrom and w:moveTo are present with paired range-marker ids, so one complete entry is a strictly stronger statement than two halves. It now also asserts the entry is Supported and carries no diagnostic. The fuzz test keeps its own-oracle half and replaces the differential half with the same signal the differential existed to catch -- a comparable case producing zero revisions -- pinned as a constant, since every case is a pure function of its seed. Corpus data outliving its tests (the WC003 row list, the expected-schema-error list) moves to files of its own. Measured: library 135 -> 133 warnings, tests 779 -> 713, suite 3904 tests all green. Suppressed nullable debt drops 3,443 -> 2,907 sites (536 retired; PackageMerge.cs carries 31 of the engine's forward).
… npm BREAKING CHANGE: the engine selector, the *WithLog comparison surfaces, and the old getRevisions shape are gone from every transport. Engine selector. --engine on the redline CLI, the `engine` int on the WASM exports, and the `ComparisonEngine` TypeScript enum all named a choice that no longer exists. Removed rather than kept as an ignored argument. Dead comparison knobs. detailThreshold tuned the removed engine's LCS granularity and --simplify-move-markup worked around its move markup; neither has a DocxDiff equivalent. They are gone from the WASM and npm surfaces. The CLI keeps both flags one release longer as an explicit warning rather than an unknown-flag error, so a script passing them tells its author what happened instead of failing. --no-detect-format-changes maps onto TrackBlockFormatChanges, which is the real equivalent. getRevisions. This was the one genuine capability gap: the legacy call read ONE already-redlined document, while DocxDiff.GetRevisions needs both sides, so it was not a drop-in. It now reads the document's own markup through DocxSession.ListRevisions, which was already wired through every transport. Consequences, all deliberate: - The move knobs (detectMoves, moveSimilarityThreshold, moveMinimumWordCount, caseInsensitive) are gone. They existed because the old call RE-DERIVED moves with a Jaccard threshold over a document that already had w:moveFrom/w:moveTo markup. The moves in the document are now simply the moves. - A move is ONE entry, not a source/destination pair, and is only emitted when both halves are present with paired range-marker ids. moveGroupId and isMoveSource had nothing left to express, so isMoveSource/isMoveDestination/findMovePair go with them. - getRevisions returns RevisionListEntry -- the shape the session's own listRevisions already returned -- and the WASM export returns DocxSessionJson.SerializeRevisionList verbatim. A DTO of its own was written first and then deleted: it would have forked one concept into two wire shapes (PascalCase families here, snake_case there), which is exactly what the single-owner rule exists to prevent. - The listing now covers families the legacy reader never saw -- rows, cells, content controls, numbering, property changes -- and each entry carries an addressable id, a family, a block anchor and a resolution status. Because the reported type is now the markup-level name (ins/del/moveFrom) rather than Inserted/Deleted/Moved, isInsertion/isDeletion/isMove/isFormatChange would have silently matched nothing. They test `family` instead. RevisionType keeps its old names for the docxDiff* APIs, which still return them. ComparisonLog and the two *WithLog surfaces are removed rather than left returning an empty log, which is what they had already been doing on the DocxDiff path. Verified: library, tests, redline CLI and WASM all build; npm tsc --noEmit clean; WASM wire size 3656 KB against a 4096 KB budget.
…dated
CHANGELOG gets the breaking entries, written for someone upgrading: what is gone,
what moved, and what silently changes for a caller who passes nothing. The last
point matters most -- DocxCompare.Compare still applies PreAcceptInputRevisions and
PreserveInputRevisions, so moving a call to DocxDiff.Compare without setting both
produces whole-document churn on revision-bearing inputs.
CLAUDE.md's numbers were stale the moment the engine went: library 135 -> 133
warnings, tests 779 -> 713, suite ~4,260 -> ~3,900, and the all-21-stripped nullable
figure 3,659 -> 2,977. The count of #nullable-disable files stays at 21 because
PackageMerge.cs replaces WmlComparer.cs in that set. Its "DocxDiff still depends on
WmlComparer's types, so the older engine is not removable" note was wrong on the
detail (DocxDiff referenced those types only in doc comments; the real dependency
was three package-merge helpers) and is replaced by what a reader now needs: that
DocxCompare and DocxDiff differ by the input-revision policy.
The four WmlComparer design docs are kept and banner-marked historical rather than
deleted -- they record Word-behaviour findings DocxDiff inherited or deliberately
diverged from, which would be expensive to rediscover.
npm tests: engine-selector.spec.ts swept a selector that no longer exists, but two
of its assertions were about the comparison itself, so it becomes
compare-front-door.spec.ts keeping those. The trim-validation spec still guards the
same reflective GetPackage() path -- that path is now PackageMerge, reached from
DocxDiff's renderer. The harness's getRevisions unwrapped a { Revisions: [...] }
envelope that is now a bare array, and docxodus.spec.ts asserted every revision
carries moveGroupId/isMoveSource; it now checks the id/family/resolutionStatus the
grouped entries actually have.
Verified: npm tsc --noEmit clean, npm run build clean (WASM wire size 3656 KB
against a 4096 KB budget).
…moval-warnings-1vkedb # Conflicts: # CHANGELOG.md
The browser suite caught a stale assertion the typechecker could not: worker.spec still checked `firstRevision.revisionType`, which was undefined. getRevisions now returns RevisionListEntry, whose markup-level kind field is `type`; `revisionType` belonged to the removed shape. The test asserted a field that no longer exists, so it failed for the right reason. It now checks id / author / type / family / resolutionStatus -- the fields the entry really carries -- rather than being narrowed until it passes. Also corrects the RevisionType enum's doc comment, which still described it as matching WmlComparerRevisionType. It matches DocxDiffRevisionType and belongs to the docxDiff* APIs; getRevisions does not use it. That distinction is exactly what this failure was about, so leaving the comment wrong would invite the same mistake. Verified: the 8 affected chromium specs pass; tsc --noEmit clean.
|
Browser suite result, as promised in the description. 659 passed, 10 skipped, 10 failed. One of those failures was real and is fixed in 299378b; the other nine are this sandbox, not the diff. The real one
Fixed by asserting what the entry actually carries — The 8 affected chromium specs pass after the fix, and The nine that aren't this PR's
Neither spec file is touched by this PR ( Generated by Claude Code |
Main's #644 changed the front door's input-revision policy in DocxCompare.cs, the same file this branch rewrites, so the collision is semantic rather than textual and the resolution is a decision, not a merge artifact. This branch preserved PreAcceptInputRevisions + PreserveInputRevisions on the front door, faithfully reproducing what the pre-v11 settings mapping did. #644 established that preserving is wrong there: Word's Compare dialog says it treats existing revisions as accepted, and the preserve behavior had been decoded from oracle documents that turned out to be Combine-shaped. That is a correctness fix and this branch was only carrying old behavior forward, so main's policy wins and this branch's structure carries it: ApplyFrontDoorRevisionPolicy now sets PreAccept alone, keeping the clone so a caller's settings object is untouched. Also taken from main: the identity shortcut normalizes a strict left to transitional on the way out. That coexists with this branch's removal of the math-run guard from CanReturnExactNoOp -- both touch the shortcut, but the guard existed only because WmlComparer's preprocessing repaired that markup as a side effect, and no engine does so now (issue #642). Tests follow the same rule: main's assertions against this branch's API. The strict self-compare pin moves from byte-preservation to detached-transitional and loses its engine Theory. One conflict git did not report: DocxDiffStrictConformanceTests.cs merged cleanly because this branch never touched it, but #644's new selector test calls the removed ComparisonEngine and WmlComparerSettings -- two compile errors the build caught. Rewritten against the front door rather than dropped, since the strict identity assertion is worth keeping. Baselines measured on the merged tree rather than picking a side: library 133 warnings, test project 716 (was 713 here and 782 on main; #644 adds test files). The CHANGELOG entry claiming the front door still preserves is corrected.
#643 removes the WmlComparer engine entirely — 81 files, ~20k lines out — and #644 is a render-fidelity round touching the converter. Both land on paths this demo exercises, so neither was taken on trust. Every engine call the demo makes still exists after the removal (docxDiffGetRevisions, docxDiffCompareProducts, convertDocxToHtml, proveRedlineReversibility, openDocxSession); the two exports that went, compareDocumentsWithLog and compareDocumentsToHtmlWithLog, are ones it never used. Nothing in docs/demo/ or the spec named WmlComparer. Rebuilt and re-ran: 14/14 browser assertions pass on the WmlComparer-free engine, including the reversibility proof and the footnote citation check. Re-measured, because #643 and #644 both touch measured paths, and the answer is that nothing moved: controlled medians came back revisions 105 -> 101, redline 140 -> 134, full+HTML 282 -> 278, compareProducts 126 -> 125, and the conversion stage in isolation 143 -> 144 ms. All inside the container's own spread. The published table is therefore left alone rather than churned by a few percent in either direction — the same restraint the README's attribution list asks for. CHANGELOG conflicted again on both sides adding under [Unreleased]; resolved keeping both, library entries first.
…entation (#677) Removing the legacy comparison engine (#643) deleted the benchmark harness's trailing summary block along with the legacy stage that preceded it. That block held the only `return` on the fall-through path of an `int`-returning top-level entry point, so `benchmarks/complex-form-doc` has not compiled since — CS0161, "not all code paths return a value". Nothing noticed for a week because the project sits outside `Docxodus.sln` and no workflow builds it. Restores the summary line and the `0`/`2` exit code, and adds a CI step that compiles every out-of-solution tool and benchmark so a library change cannot rot one silently again. All eight such projects build clean in Release today. The same removal left `README.md` and `FINDINGS.md` promising a `WmlComparer` stage, two expected `[check] legacy ... FAIL` lines, and an exit-code story that the harness could not produce. The README is rewritten against what the harness actually runs: its stage table is now keyed on the exact `Bench(...)` labels, the exit codes are spelled out (including that a throwing stage is caught and counted rather than propagated), and it points at the in-repo `TestFiles/NVCA-Model-COI.docx` fixture, which is committed despite the old text saying otherwise. `FINDINGS.md` is refreshed from a recorded run of this commit — exit 0, all eight checks passing — with the input digest, runtime, and command captured, and with stable assertions separated from one-run timings and revision counts. The legacy engine's measurements move to a clearly-marked historical section explaining why that engine was retired. `ComplexFormBenchmarkContractTests` ties the two together: it asserts the README and `Program.cs` name the same set of stages, in both directions, plus a vacuity guard so an empty match cannot pass silently. Against the pre-fix README it fails three of its four assertions. Closes #662 Closes #669
Removes
WmlComparer, the legacy comparison engine, and carries that through every surface that touched it. Breaking, for v11.0.0.DocxDiffhas been the default since v8.0.0. This deletes the alternative rather than keeping a second, feature-frozen engine alive behind a selector.What this actually cost, before the diff
The four parity scoreboards checked
DocxDiffby running it head-to-head againstWmlComparerover the WC corpus. They could not outlive their own oracle, so the first commit freezes what they prove while both engines still exist. That is deliberately a separate commit, so the evidence lands before anything is deleted.Two artifacts, doing different jobs:
docs/architecture/wmlcomparer_parity_baseline/— the four reports verbatim from their final run against a liveWmlComparer. A historical record; nothing can re-derive it now. It answers "was the IR engine ever actually checked against the thing it replaced, and on what": 179 runnable cases at 177 pass / 2 documented deviations, 39 markup cases all pass, 84 Consolidate cases all reproduce, and 184 differential comparisons at 117 MATCH / 18 GRANULARITY / 47 catalogued DIVERGENT / 2 pairs the legacy engine threw on andDocxDiffhandled.DocxDiffCorpusBaselineTests+DocxDiffCorpusBaseline.tsv— the live replacement. Same 92 pairs, both directions, both granularities (368 rows), pinningDocxDiff's own per-kind multiset of normalized revision text. Verified 368/368 reproduce. It pins the multiset rather than a count, so a regression that swaps which text is inserted while keeping the tally still fails. Four whole-document-rewrite rows collapse to a SHA-256 over the same encoding (they were 728 KB each; the median row is 93 bytes), which took the file from 1.65 MB to 48 KB without weakening it.What was kept
The cross-package merge helpers —
CopyMissingStylesFromOneDocToAnother,CopyMissingNumberingFromOneDocToAnother,MoveRelatedPartsToDestinationand their private helpers — move verbatim toDocxodus/PackageMerge.cs. They copy styles, numbering definitions and related package parts between two packages; none of it is comparison logic, andDocxDiff's markup renderers have always called them. The move is byte-for-byte with the#nullable disableheader intact, so a regression there can only come from a later edit rather than from the extraction.This corrects a claim in
CLAUDE.md: it saidDocxDiffdepends onWmlComparer's types, so the engine was not removable.DocxDiffreferenced those types only in doc comments. The real dependency was these three helpers, which the compiler found immediately.The one thing most likely to break a caller silently
DocxCompare.Comparealways appliesPreAcceptInputRevisions; the rawDocxDiffAPI leaves it opt-in. That is the difference between Word-like behaviour and whole-document churn on revision-bearing inputs, and it used to be buried in a settings mapping that no longer has a reason to exist. It is nowApplyFrontDoorRevisionPolicy, applied to a clone so a caller's settings object is never mutated, and pinned by tests. Anyone moving a call fromDocxCompare.ComparetoDocxDiff.Comparemust set that flag to keep their current output.This branch originally applied
PreAcceptandPreserveInputRevisions, faithfully reproducing the pre-v11 mapping. #644 landed onmainwhile this was open and established that preserving is wrong on this surface — Word's Compare dialog says it treats existing revisions as accepted, and the preserve behaviour had been decoded from oracle documents that turned out to be Combine-shaped. That is a correctness fix and this branch was only carrying old behaviour forward, so the merge takes main's policy and keeps this branch's structure. Preservation remains an explicit opt-in on the raw API.getRevisions: the one genuine capability gap
The legacy call read ONE already-redlined document;
DocxDiff.GetRevisionsneeds both sides, so it was not a drop-in. It now reads the document's own markup throughDocxSession.ListRevisions, which was already wired through every transport. Consequences, all intended:detectMoves,moveSimilarityThreshold,moveMinimumWordCount,caseInsensitive) are gone. They existed because the old call re-derived moves with a Jaccard threshold over a document that already carriedw:moveFrom/w:moveTomarkup. The moves in the document are now simply the moves.moveGroupId,isMoveSource, and theisMoveSource/isMoveDestination/findMovePairhelpers go with them.getRevisionsreturnsRevisionListEntry— the shape the session's ownlistRevisionsalready returned — and the WASM export returnsDocxSessionJson.SerializeRevisionListverbatim. A dedicated DTO was written first and then deleted: it would have forked one concept into two wire shapes (PascalCase families on one path, snake_case on the other), which is what the single-owner rule exists to prevent.ins/del/moveFrom),isInsertion/isDeletion/isMove/isFormatChangewould have silently matched nothing. They testfamilyinstead.A capability that is genuinely lost
Some older Word documents put tracked-revision wrappers inside an Office Math run — schema-invalid markup that
WmlComparer's preprocessing repaired as a side effect.DocxDiffdoes not repair it. Measured onWC012-Math-After.docx: source has one validation error,DocxDiff's output has the same one error, and the bytes do not change.So the guard in
CanReturnExactNoOpthat refused the byte-identical shortcut for such documents was buying nothing but a full comparison returning the same bytes. It is removed,CanReturnExactNoOpis now plain byte equality, and a test pins the honest behaviour: invalid input passes through unrepaired rather than being silently rewritten. Tracked as #642 rather than left implicit. (The strict-OOXML normalization #644 added to that same shortcut is independent and is kept — a strict left is still normalized to transitional on the way out.)Tests
The 73
WmlComparer-specific tests go. Tests that used the engine only to synthesize a redline fixture now useDocxCompare. Two needed real thought:Movedrevisions. One grouped entry is the correct equivalent, so it now also asserts the entry isSupportedand carries no diagnostic — otherwise weakening the count would have been indistinguishable from hiding a regression.The fuzz test keeps its own-oracle half and replaces the differential half with the same signal that half existed to catch — a comparable case producing zero revisions — pinned as a constant, since every case is a pure function of its seed. Corpus data outliving its tests (the WC003 row list, the expected-schema-error list) moves to files of its own. The four
WmlComparerdesign docs are banner-marked historical rather than deleted; they record Word-behaviour findingsDocxDiffinherited or deliberately diverged from.Two problems surfaced only by actually running things, not by the compiler:
worker.spec.tsassertedfirstRevision.revisionType, a field that no longer exists onRevisionListEntry(the kind istype). TypeScript could not catch it because the assertion runs insidepage.evaluateon anany. Fixed by asserting the fields the entry really carries —id/author/type/family/resolutionStatus— rather than narrowing the test until it passed.mainbrought inDocxDiffStrictConformanceTests.cscleanly — git reported no conflict, because this branch never touched that file — but Render-fidelity round: package tolerance, section defaults, style provenance, and Word-Compare input-revision policy #644's new selector test calls the removedComparisonEngineandWmlComparerSettings. Two compile errors inside a file git considered resolved. Rewritten against the front door rather than dropped, since the strict-identity assertion is worth keeping.Verification
Measured on the current head,
3caeecc(the merge ofmain@ e605d62). "Before" ismain.NoWarnsites (CS8073/CA2200/CS8632)536 nullable warnings retired — 16% of the total;
PackageMerge.cscarries 31 of the engine's forward, and the opt-out file count stays at 21 because that file inherited the header. TheNoWarnlist barely moves: of the 4 sitesWmlComparerheld, 1 came back withPackageMerge.cs, so all three codes stay suppressed. The remaining nullable debt is now tracked in #645 and its sub-issues (#646–#651).Also verified:
redlineCLI builds; WASM builds and publishes at 3656 KB wire size against a 4096 KB budget;npm run buildandtsc --noEmitclean.Closes nothing on its own; #642 tracks the math-run repair.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XvfkMu2yXDS41AQ7256YxT