fix(docx-compare): align identical text across run re-segmentation - #1148
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c002df6914
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (current.localName === 't') { | ||
| text += current.textContent ?? ''; |
There was a problem hiding this comment.
For paragraphs where a word is split across multiple w:t leaves, concatenating the leaves before tokenization changes the public metric unit—for example, t + he was two tagged-token-v1 atoms and is now one—while deriveTaggedTreeStats still reports atomMetricVersion: 'tagged-token-v1'. This makes persisted metrics and consumer thresholds silently incomparable and violates the active docx-comparison requirement that a weighting change use a new version value; either retain the v1 weighting or introduce and publish a new metric version.
AGENTS.md reference: openspec/AGENTS.md:L456-L456
Useful? React with 👍 / 👎.
| for (const value of tokenizeComparisonText(group.map(runText).join(''))) { | ||
| tokens.push({ value, run: ownerAt(offset), start: offset }); |
There was a problem hiding this comment.
When adjacent same-rPr runs in a mixed-format replacement gap belong to separate revisionAttributionRanges but together form one token, this assigns the whole token only to the run owning its first character. The later insertion/deletion path reads provenance solely from that run, so an operation attached to a subsequent run emits no attributed revision and resolveTaggedRevisionAttributions throws operation ... has no emitted attributed revision; group on operation provenance as well, or split side-only tokens at source-run boundaries before wrapping them.
Useful? React with 👍 / 👎.
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)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
c002df6 to
3c6d9bf
Compare
Peer review, round 1 (Codex,
|
Peer review, round 2 (confirmation, Codex)Verdict: CHANGES_REQUIRED. The round-1 fixes were confirmed: A/B attribution resolves, a pure move is 0/0, and move plus replacement and move plus deletion match main. The full package suite (817), the tagged suite and the real-corpus suite (30/30) passed. The reviewer found two new regressions caused by my round-1 whitespace change. Adjudication
Also added: an unaligned gap whose two sides spell the same unmoved text (for example Every case in the reviewer's probes now matches main: both of its regression assertions pass; the whitespace deletions, the replacement and the space-dominance probes give 2/2, 1/1 and 2/2; the hyperlink, tab, break and space moves give 0/0. The Voting Agreement fixture stays at 1/1 and the merge-only control at 0/0. Unit tests cover all of these. The manifest is unchanged from round 2 (ILPA atoms 2926/1385). Fixes are in 4308a19, and a confirmation round follows. |
A mixed-formatting paragraph tokenized each run on its own, so the same text split into runs differently (`)` + `,` against `),`) produced spurious deletion/insertion pairs around unchanged punctuation. A safe-docx save of a one-word edit in the NVCA Voting Agreement preamble reported 7 deletions and 4 insertions instead of 1 and 1. Tokenize each maximal group of adjacent runs that share a run-property signature as one text stream, reusing the concatenated-path token/offset machinery; a formatting change still forces a token boundary. Readable whitespace bridges (#998) stay limited to single-signature gaps so they never coalesce across a formatting change. The tagged-token-v1 atom statistics tokenized each w:t on its own and disagreed with the range counts (3/6 atoms for a 0/0-range re-segmentation). Atom keys now tokenize adjacent w:t text as one stream per paragraph. The strategy-differential manifest is updated: ILPA loses 23/18 spurious insertion/deletion ranges, and atom counts drop where words were split across runs. Closes #1142
Peer review of the run re-segmentation fix found two regressions: - Adjacent same-format runs attributed to different operations were concatenated into one token, so publication failed with "operation B has no emitted attributed revision". Tokenization groups now also break where operation provenance changes. - Paragraph atom alignment over one concatenated text stream merged the spaces on either side of moved or removed runs (`prefix ` + `cat` + ` suffix` against `prefix suffix`), so a pure inline move reported 3/3 atoms and deleting a word run reported extra whitespace atoms. Moved subtrees now keep their own token boundaries before their standalone weight is subtracted, and paragraph alignment compares whitespace per character while still weighing each run of unaligned whitespace as one atom. Ref: #1142
The confirmation review found that aligning whitespace one character at a time let a long space run outweigh unchanged words (moving ten spaces across a sentence reported 5/4 atoms instead of 2/1), and that a space deleted next to a moved run could be absorbed when the moved run's weight was subtracted. Paragraph atom streams now keep whitespace as whole tokens, split only where a w:t boundary falls inside one, so the spaces on either side of a removed or moved run stay two tokens; word and punctuation tokens still span runs. Alignment weighs matches involving moved content at half an unmoved match, so moved keys stay unaligned and are subtracted exactly. An unaligned gap whose two sides spell the same unmoved text (a double space split by a run boundary on one side only) counts as unchanged. Ref: #1142
The round-3 review showed that splitting whitespace tokens at w:t boundaries still left atoms when the same spaces were split differently (`a ` + ` b` against `a ` + ` b`), and that a deleted paragraph's weight depended on its run boundaries. Whitespace in a paragraph stream is now one key per character of each concatenated whitespace token, so keys depend only on the text. Word and control matches outweigh every whitespace match, so spaces never displace an unchanged word, and the unaligned characters of one whitespace token weigh one atom together. Whole-element weights use the plain concatenated tokens. A shortened space run now counts as one deletion rather than a deletion plus an insertion. Ref: #1142
4308a19 to
02e2417
Compare
Peer review, round 3 (confirmation, Codex)Verdict: CHANGES_REQUIRED. Both round-2 regressions were confirmed fixed. The full package suite (818), the tagged suite and the real-corpus suite (30/30) passed, and a 1560-pair sweep of the gap-cancellation rule found no hidden text change. Adjudication
I rebased onto origin/main (CHANGELOG conflict only) and reran the full gate on the rebased head 02e2417. Build, lint, the allure checks, the docx-compare, docx-core, docx-mcp and docx-markdoc suites, and the real corpus (30/30) all pass. Manifest: ILPA atoms 2932/1385; ranges unchanged at 821/740. A confirmation round follows. |
Peer review, round 4 (confirmation, Codex)Verdict: CHANGES_REQUIRED. The round-3 blocking cases are confirmed fixed (boundary shift 0/0, paragraph-deletion weight 3/3). The reviewer accepted the per-character whitespace metric (" Adjudication
A confirmation round follows to adjudicate the scoping. |
Peer review, round 5 (confirmation, Codex)Verdict: APPROVE. "Blocking: None. Your scoping argument holds… I withdraw the round-4 blocker under the revised scope." The reviewer verified this by running both builds: the #1154 repro gives identical statistics on the merge base and on HEAD (1/2 ranges, 0/0 atoms); the NVCA Voting Agreement fixture goes from 4/7 ranges and atoms on the base to 1/1 on HEAD; shortening a space run counts one deleted atom. The full docx-compare suite (818), the real-corpus suites including both new NVCA cases and the manifest, workspace lint and the conformance checks all passed. One non-blocking nit, applied: the CHANGELOG sentence "a re-segmentation-only change now reports zero atoms as well as zero ranges" overstated the range guarantee. It now reads "zero atoms, and zero ranges unless an unchanged run is matched at a different text offset (#1154)" (CHANGELOG only, in the head commit after 231f3ea). Review summary across rounds: round 1 found an operation-attribution regression and move-atom miscounts (fixed); round 2 found whitespace dominance and a move-subtraction edge (fixed); round 3 found incomplete whitespace segmentation invariance (fixed with per-character whitespace keys, which changes how whitespace-only edits are weighted; accepted by the reviewer); round 4 raised a pre-existing tree-matching case (scoped out to #1154 and the claims narrowed); round 5 approved. Follow-ups: #1149, #1150, #1153, #1154. |
The real-corpus suites are excluded from the default run, so the new fixture helper counted as uncovered and dropped docx-compare below its coverage ratchet. Exercise it on a synthetic preamble-shaped package: adjacent same-format plain runs merge across punctuation, a tab run and bookmark boundaries are respected, only the target word changes, and the comparison reports 1/1 for the edit and 0/0 for re-segmentation alone. Ref: #1142
Post-merge smoke (origin/main @ aca5f2b)I built
The extra SPA deletion range is an empty run inside a REF field that the safe-docx save dropped. Main produced the same output before this PR; it is tracked in #1150 and does not come from this change. Result: PASS. The Voting Agreement case from #1142 now yields one deletion and one insertion, and no punctuation is deleted and re-inserted. Vercel status on |
This branch was successfully deployed
1 participant
Closes #1142
Problem
refineSimpleRunGaponly tokenized concatenated text when every run in the gap shared one run-property signature. Any mixed-formatting paragraph fell back to tokenizing each run on its own, so identical text split into runs differently ()+,in two runs against),in one) produced different token sequences and spuriousw:del/w:inspairs. A safe-docx save of a one-word edit re-segments the NVCA Voting Agreement preamble (18 runs to 5), and the redline showedDEL ) DEL , INS ),twice andDEL ) DEL . INS ).once next to the real edit.Separately, the
tagged-token-v1atom statistics tokenized eachw:tindependently, so a re-segmentation-only change reported 0/0 ranges but 3/6 inserted/deleted atoms.Fix
taggedTreeSerializer.ts):tokenizedRunsnow tokenizes each maximal group of adjacent runs with the same run-property signature and the same operation provenance as one concatenated string. Token boundaries are forced where either changes, so no token mixes formatting orrevisionAttributionRangesoperations. Tokens keep the paragraph-level offset (start) and the run that owns their first character (run), the same contract the old concatenated path had, soemitCommonToken(which splits a common token at every run boundary on both sides and emits per-fragmentemitCommonRunproperty deltas) and therunFragment/provenance lookup for deleted/inserted tokens are reused unchanged. When the whole gap has one signature, the output is byte-identical to the old concatenated path.bridgeMatchesstill requires one signature across the whole gap. This is deliberate. The docx-markdoc: Markdoc builds still need raw-OOXML post-processing (tables, side stories, greenfield, formatting, readable redlines) #998 contract says readable grouping leaves formatting boundaries intact. Applying it per signature group would let a deletion/insertion chain coalesce across a bold/plain boundary (see the newkeeps readable whitespace bridges to gaps with one formatting signaturetest), and that would be a policy extension, not a bug fix. Single-signature gaps behave exactly as before. The line itself is unchanged, which keeps the diff small near the docx-markdoc: Markdoc builds still need raw-OOXML post-processing (tables, side stories, greenfield, formatting, readable redlines) #998 code.taggedTreeShadow.ts):comparisonAtomKeysnow treats adjacentw:ttext within a paragraph as one stream, so the keys depend only on the text and not on run boundaries. Any other leaf (tab, br, field chars,delText, …) and every paragraph boundary end the stream. The atom metric does not carry formatting (that isformatChangeAtoms), so it is segmentation-invariant rather than signature-grouped. When a paragraph exists on both sides, its alignment (unalignedParagraphAtoms) compares whitespace one character per key, which keeps the spaces around a removed or moved run (prefix+Company+suffixagainstprefix suffix) from turning into a different token. Any word or control match outweighs every whitespace match, so spaces never displace an unchanged word. The unaligned characters of one whitespace token weigh one atom together. A match involving moved content weighs half an unmoved one, so the existingsubtractMovessubtraction stays exact. Whole-element weights use the plain concatenated tokens. One intended difference from main: shortening a run of spaces counts as one deleted atom, not one deleted plus one inserted.Results
NVCA Voting Agreement, orig vs the safe-docx-saved
Company→SMOKEWORDrevision:The redline for that paragraph is now only
DEL Company/INS SMOKEWORD.Strategy-differential manifest changes (reviewed, all reductions in spurious churn)
checked-in/ILPA: ranges 844/758 → 821/740. All 10 paragraphs whose redline changed lose re-segmentation churn (for exampleDEL Act INS Ac INS t→ nothing, andDEL If any DEL Limited INS If INS any INS Limit INS ed→DEL If any INS If any). Atoms 3662/2101 → 2932/1385.split-run-boundary-change(t+hewas 2 atoms, now 1),p-unit-agreement-v2, and the NVCA paragraph-deletion rows for COI, Indemnification, MRL and SPA.Tests
taggedTreeSerializer.test.ts: newmixed-format run re-segmentation (#1142)block with minimal XML reproductions, run in both directions: a one-word edit with)+,vs),(exactly one del and one ins, plus stats), re-segmentation alone (0 ranges and 0 atoms), a formatting change at a run boundary next to a text edit (w:rPrChangestill emitted), a formatting boundary that splits a word (still a token boundary), the readable-bridge gate, adjacent runs attributed to different operations (separate revisions, attribution resolves), atom weights around a removed word run, a pure inline move, a move plus a deletion, and a space deleted next to a moved run, plus whitespace edits (a moved space run, the same spaces split differently by run boundaries, and a deleted paragraph's weight under two segmentations). The first three fail on main. The attribution and atom tests cover regressions that review found in this PR's earlier commits.real-corpus-paragraph-deletion.test.ts): a newreal-corpus run re-segmentationblock derives the save-style re-segmentation of the Voting Agreement preamble (merging adjacent same-rPrplain text runs) with and without the one-word edit. It asserts 1/1 and 0/0 ranges and atoms. Both fail on main.npm run test:real-corpus -w @usejunior/docx-comparelocally against the SHA-verified corpus (scripts/prepare_real_comparison_corpus.mjs,*_REQUIRED=1): 30/30 passed.npm run build,lint:workspaces,check:allure-labels,check:allure-quality,check:allure-filenames, and thetest:runsuites for docx-compare, docx-core, docx-mcp and docx-markdoc all pass.Known gaps (not changed here)
modifications/modifiedParagraphsstill reports 1 for a re-segmentation-only paragraph, because it is derived from tagged-tree node tags and not from the refined output. Follow-up: docx-compare: modifiedParagraphs counts a paragraph whose runs were only re-segmented #1149.formatChangesdoes not count run-level property deltas emitted inside a refined text gap (docx-compare: formatChanges stays 0 when a run changes both text and formatting, although w:rPrChange is emitted #937, pre-existing).