Record note and rule insertion as tracked changes so the redline can be rejected - #625
Merged
Merged
Conversation
…an be rejected InsertFootnote/InsertEndnote ignored TrackedChangeMode.RenderInline entirely. The citation and the note definition were both written as ordinary content, so a reviewer had nothing to accept or reject and reject-all left the note behind. The redline looked complete and only failed when somebody actually rejected it. The citation is now the reversible unit and the definition follows it: the body reference run is wrapped in w:ins, rejecting removes it, that leaves the note uncited, and the note-lifecycle rule deletes it in the same resolve. The definition carries no revision markup of its own on purpose -- a w:ins inside it would be a revision with no independently meaningful resolution. That rule -- a note definition exists exactly as long as something still cites it -- now has one owner in Internal/NoteReferenceOps.cs, shared by DocxSession's resolve paths (#516, #591) and by RevisionProcessor's stateless reject path, which is what every non-.NET transport reaches through DocxDiffOps.RejectRevisions. Without that the same redline was reversible in-session and not reversible through npm/python/MCP. The stateless accept path deliberately does not apply it: Accept(Compare(l, r)) == r is the comparison engine's contract, and a counterpart carrying a reference-less definition is entitled to keep it. Consolidating also fixed the rule's scope -- it now asks the whole package who cites a note rather than only the body, so a note cited from a running header survives losing its body citation. InsertHorizontalRule had the same defect with a smaller blast radius and takes the same paragraph marking InsertParagraph already applies. Closes #614
JSv4
pushed a commit
that referenced
this pull request
Aug 31, 2026
#625 fixed the defect this demo found (#614): InsertFootnote under render_inline recording wrote the citation and the note definition as ordinary content, so reject-all removed neither and the note's text survived a "full" rejection. Act II had been carrying a plain paragraph in place of the footnote it wanted, with a comment and a guard test pinning the workaround. Both come out. The marker goes at the end of the clause rather than against the figure it qualifies, and that is the engine's constraint, not a style choice. The "1.5x" text was written moments earlier as a tracked insertion, and citing into it nests a reference run inside a w:ins — a revision with no independently meaningful resolution. InsertFootnote refuses that outright ("offset falls inside a revision or unsupported inline container"), which is the right answer and cost one iteration to discover. So the script searches the clause's untouched tail and cites past it, which is also where Word convention puts a marker. That needed an offset the script does not know up front, so a step can now bind one: `bindOffset` stores the character just past a search match, and `offsetAfterMatch` is the arithmetic. It keeps the demo's rule that an agent looks everything up rather than hardcoding it — a literal offset would drift the moment any earlier act changed the sentence. The guard test is inverted rather than deleted: the script must still contain exactly one note-insertion beat, because the reversibility proof running over a footnote on every page load is the end-to-end check that #625 works through the wire, and a silent removal would take it away. A new browser assertion reads the citing paragraph's XML out of the redline and requires the footnoteReference to sit inside a w:ins, so the proof cannot pass vacuously by never writing a note; the reject-all test gained the matching negative, that the side-letter text is gone. Verified: the proof panel reads REVERSIBLE with zero content differences, 26 revisions across 3 authors, 31 tool calls at a 5.1 ms median. 14/14 browser assertions and 61/61 node checks pass. Also retires the known-gap section in tracked_changes.md, which is now false — it points at the encoding docs #625 added and keeps only the account of how the closed-set classification surfaced it. Republished figures after #626. Measured the same way the table is read, three full 40-frame runs per depth on one container: revisions 152 ms (was 144, inside the documented noise band), redline 181 ms (was 206), full+HTML 352 ms (was 422). The ratio against the mutation path is now 60x to 154x, so the loop runs between 3 and 7 fps rather than 2 and 6. The compareProducts-vs-two-calls pair, measured the controlled way (fixed inputs, medians of nine, three reps), is 126 ms against 196 ms. The two deeper rows moving 10-18% while the shallow one held is what a load-path change looks like, though the panel measures the total, not the attribution.
This was referenced Aug 31, 2026
JSv4
pushed a commit
that referenced
this pull request
Aug 31, 2026
#638 completes what #625 started on the path this demo exercises. #625 made the citation the reversible unit but left the definition unmarked, compensating with an unconditional prune on the stateless reject. That made the redline ambiguous to a stateless consumer: a w:ins citation beside an unmarked definition is also exactly what a comparison emits when the counterpart merely cites a husk the baseline already owned, and the unguarded prune ate that husk. Now both halves record and the prune is guarded on the definition being emptied as well as orphaned. The demo's comments and the tracked_changes.md pointer both described the #625 mechanism, so both were describing an encoding the engine no longer writes. Corrected. The browser assertion is extended rather than left alone, because the old one would still pass against the ambiguous shape: it checked the citation run sits inside a w:ins and stopped there. It now also pulls the note definitions out of the redline and requires the one Act II authored to carry insertion markup. Identifying it by its text matters — the part also holds Word's two reserved separator notes, which carry no markup and must not. No re-measurement. #634 and #638 are on the note-lifecycle and reject paths; the stress loop times docxDiffGetRevisions and docxDiffCompareProducts, which neither touches. Published figures stand as measured at #627. Verified on the rebuilt engine: 14/14 browser assertions, 61/61 node checks. Also merges #633 and #637, both test-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #614.
What was actually wrong
The issue describes the citation as being wrapped in
w:insand the definition as staying behind. It is worth correcting the mechanism, because the next reader will otherwise go looking for aw:insthat was never there:InsertFootnote/InsertEndnotenever consulted the recording mode at all. Both the body citation and the note definition were written as ordinary content, sorejectAllRevisionshad nothing to reject — which is why the reproduction reports two residual revisions rather than one. Dumped from the real op before any change:The consequence the issue names is exactly right, and it is the bad one: the redline looks complete and only fails when somebody actually rejects it.
The encoding
The citation is the reversible unit and the definition follows it. Rejecting the
w:inscarries the reference away, which leaves the definition uncited, and the note-lifecycle rule deletes it in the same resolve. Accepting unwraps thew:insand both survive.The definition deliberately carries no revision markup of its own. A second
w:insinside it would be a revision with no independently meaningful resolution: rejecting it while keeping the citation yields an empty note, and keeping it while rejecting the citation yields a note that is pruned anyway.This is option 1 from the issue rather than the fail-closed fallback, because the mechanism it needs was already in the codebase —
PruneOrphanedNotes, added for #516 and extended for #591 — and pointing the op at it costs less than refusing does.One rule, one owner, and one place it was wrong
Internal/NoteReferenceOps.csis now the single owner of a note definition exists exactly as long as something still cites it. Two callers:DocxSession, after a structural delete or a revision resolution, in both directions. Its previous private copy moved here unchanged in behaviour except for one thing — it scanned onlyword/document.xmlfor citations. A note cited from the body and a running header was therefore "referenced before, unreferenced after" when the body citation went away, and got deleted out from under the header. It now asks every story a note reference can legally live in.DS431covers it.RevisionProcessor, on the reject path only. This is the gap that made the fix incomplete without it:DocxDiffOps.AcceptRevisions/RejectRevisionsroute toRevisionProcessor, and that is what npm, python, the MCP server and WASM call. Before this change the same redline was reversible through aDocxSessionand not reversible through any of those transports.Why reject only, and not accept. I tried it symmetrically first and it broke three
DocxDiffScenarioTestscases, which turned out to be the right answer arriving as a test failure.Accept(Compare(l, r)) ≡ ris the comparison engine's documented contract, and the fixtures' counterpart documents legitimately carry a reference-less note definition — a naive paragraph deletion leaves one. Accept must reproduce the counterpart as the comparison saw it, husks included. Reject cannot hit that conflict: a note loses its citation on reject only when the redline introduced that citation, in which case the definition came in with the redline too.DocxSessionapplies the rule in both directions because an editor is authoring a document rather than inverting a comparison.DS429/DS430pin the two contracts as distinct so a future symmetry cleanup fails loudly.The audit the issue asked for
Every session op that writes package state under recording now falls in one of three rows, and
docx_mutation_api.mdcarries the table:ReplaceTextfamily, the delete family,InsertParagraph,SplitParagraph,MergeParagraphs,MoveBlock, the formatting ops, the table row/column ops,InsertHorizontalRule(fixed here),InsertFootnote/InsertEndnote(fixed here)tracked_operation_unsupportedInsertTable,ApplyListFormatRange, every image mutationSetHeaderText,SetFooterText,InsertPageNumberField,SetPageNumbering, …). Word does not redline header/footer authoring either. Comments and annotations are likewise not revisionsImages were already fail-closed, so the issue's other named suspect was clean.
InsertHorizontalRulewas not: it wrote its paragraph untracked, so rejecting left the rule behind. It now takes the sameMarkParagraphContentAndMarkcallInsertParagraphalready makes. The documentation that claimedInsertParagraphdoes not track was simply stale and has been corrected.Making header/footer authoring trackable is feature-sized and is not attempted here; the table says so plainly rather than leaving the reader to infer that the gap is closed.
Validation
Eight new test cases, all of which fail on
main:DS366(footnote and endnote) — author under recording, then resolve both ways. Reject leaves only Word's two reserved separator notes, no citation, andDocxDiff.GetRevisions(baseline, rejected)is empty, which is the property that actually matters. Accept keeps citation and definition with now:insleft.DS367— the diff engine still reports the insertion. The citation is aw:footnoteReferencenested insidew:ins, a shape this op had never produced.DS368— whatproveRedlineReversibilityreports, which is how the issue was found. The reject path still diverges indocument.xml(the run the citation split),settings.xmlandstyles.xml— residue a generated redline legitimately explains — but/word/footnotes.xmlis no longer among them. The assertion reads a populated divergence list, so it is not vacuously true.DS429/DS430— the stateless transport path in both directions, pinning the asymmetry above.DS431— a note cited from a header survives losing its body citation.DS224— the horizontal rule is a revision, and rejecting it takes the paragraph back out.Full suite: 4183 passed, 0 failed, 3 skipped. Warning baselines move by exactly one each (132 → 133 library, 775 → 776 tests) for the one new file, per the rule in
CLAUDE.md; both numbers are updated in the same commit.Note for whoever lands #615
That branch carries a guard asserting
insert_footnoteis not reversible under recording:It is now stale. The demo can footnote the negotiated liability cap again if that reads better than the paragraph it was changed to.