From b4060adb68e0ad60a198dbd72728d0087e97cd1e Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sun, 30 Aug 2026 23:44:40 +0200 Subject: [PATCH 01/14] fix: author-filtered accept_all/reject_all resolve only that author's elements (ROADMAP.md #75) accept_all(author=A) and reject_all(author=A) selected revisions by author but resolved them by w:id against an index holding every author's elements. Where a producer repeats a w:id across authors (LibreOffice does, and the corpus has such files), the id lookup returned the other author's element: reject_all(author="A") deleted B's inserted text and accept_all(author="A") made it permanent, both reporting 2. _revision_element_index gains an optional author parameter and _resolve_all passes it, so a filtered call has no other author's element to land on. The filter uses list_revisions' own author expression -- a missing w:author reads as "Unknown" -- so the index and the listing cannot disagree about who owns an element, which is what would otherwise turn the fix into a silent no-op. The group/changeset path (_resolve_ids) needs no author scoping: a member id is never a duplicated one, since _reconstruct_groups bars every occurrence of a repeated w:id from every group and _seed_max_change_id keeps a freshly allocated id above every id already in the file. --- docx_editor/track_changes/base.py | 2 +- docx_editor/track_changes/listing.py | 24 +++++++++++++++----- docx_editor/track_changes/resolution.py | 30 +++++++++++++++++-------- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/docx_editor/track_changes/base.py b/docx_editor/track_changes/base.py index 582cc18..a80a56b 100644 --- a/docx_editor/track_changes/base.py +++ b/docx_editor/track_changes/base.py @@ -38,7 +38,7 @@ class _RevisionManagerBase: _defer_range_sweep: bool _range_sweep_pending: bool - def _revision_element_index(self) -> dict[str, list[Element]]: + def _revision_element_index(self, author: str | None = None) -> dict[str, list[Element]]: raise NotImplementedError def _is_in_document(self, elem) -> bool: diff --git a/docx_editor/track_changes/listing.py b/docx_editor/track_changes/listing.py index 5e4a7f1..1d83da8 100644 --- a/docx_editor/track_changes/listing.py +++ b/docx_editor/track_changes/listing.py @@ -272,7 +272,7 @@ def _parse_revision( changeset_source=self._changeset_sources.get(changeset_id) if changeset_id is not None else None, ) - def _revision_element_index(self) -> dict[str, list[Element]]: + def _revision_element_index(self, author: str | None = None) -> dict[str, list[Element]]: """Map ``w:id`` -> its handled revision elements, in one recursive walk. ``HANDLED_REVISION_TAGS`` only: these are the revision elements @@ -292,10 +292,17 @@ def _revision_element_index(self) -> dict[str, list[Element]]: changeset members never collide (``_reconstruct_groups`` bars every duplicated id from every inferred group, and our own allocator keeps recorded ids unique), so for those callers every list holds exactly one - element. Whole-document resolution (``_resolve_all``) is where the - duplicates live: keeping them all lets one index serve every same-id - element, so they resolve within a single pass instead of costing a - rebuilt index and a whole extra pass each. + element. Unfiltered whole-document resolution (``_resolve_all`` with no + author) is where the duplicates live: keeping them all lets one index + serve every same-id element, so they resolve within a single pass + instead of costing a rebuilt index and a whole extra pass each. + + ``author`` scopes the index to that author's elements, which is what + makes an author-filtered ``_resolve_all`` unable to reach another + author's same-id revision: the lookup has no other candidate to + return. The author test is ``list_revisions``'/``_unhandled_elements``' + — a missing ``w:author`` reads as ``"Unknown"`` — so the index and the + listing cannot disagree about who owns an element. Each id's list is in document order — the same order the fresh scan in ``_find_revision_element`` uses, so a duplicated id resolves the @@ -303,6 +310,8 @@ def _revision_element_index(self) -> dict[str, list[Element]]: """ element_index: dict[str, list[Element]] = {} for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): + if author is not None and (elem.getAttribute("w:author") or "Unknown") != author: + continue element_index.setdefault(elem.getAttribute("w:id"), []).append(elem) return element_index @@ -337,7 +346,10 @@ def _find_revision_element( Returning the *first still-attached* candidate (rather than a single remembered element) is what lets duplicate ids resolve from one index: once an element is detached its successor becomes the answer, so N - same-id revisions resolve in one pass rather than N. + same-id revisions resolve in one pass rather than N. Which elements are + candidates at all is the index's business: an author-scoped index + (``_revision_element_index(author)``) holds only that author's, so a + filtered call cannot land on another author's same-id revision. """ if element_index is None: wanted = str(revision_id) diff --git a/docx_editor/track_changes/resolution.py b/docx_editor/track_changes/resolution.py index 802ed8b..dd4d902 100644 --- a/docx_editor/track_changes/resolution.py +++ b/docx_editor/track_changes/resolution.py @@ -154,6 +154,13 @@ def _resolve_ids( ``_is_in_document`` (inside ``resolve``) treats a detached member as already gone. + The index needs no author scoping: a member id is never a duplicated + one. ``_reconstruct_groups`` leaves every occurrence of a repeated w:id + wholly ungrouped, and a freshly allocated id is above every id already + in the file (``_seed_max_change_id``/``_fold_revision_ids`` fold them + all into a high-water mark), so no member can share its id with a + second element — of this author or any other. + Termination rests on ``resolve`` returning False once an id has no live element left. accept_all/reject_all cannot use this loop — they resolve every id in the document, where Word may repeat a w:id across authors; @@ -273,8 +280,13 @@ def _resolve_all(self, author: str | None, resolve: Callable[[int, dict[str, lis same id back repeatedly. * **The index maps each id to every element carrying it.** Word may reuse one w:id across authors (A's w:ins and B's w:del both id=7); - ``_find_revision_element`` returns the first still-attached - candidate, so same-id revisions resolve within one pass. + unfiltered, ``_find_revision_element`` returns the first + still-attached candidate, so same-id revisions resolve within one + pass. A filtered call instead indexes only ``author``'s elements + (``_revision_element_index(author)``), so another author's element + sharing that id is never a candidate: the listing and the index + agree on ownership, and the call resolves exactly the rows it + listed. The index is built once for the whole call, not per pass: resolution only ever detaches elements, so a single build stays a valid superset @@ -284,7 +296,7 @@ def _resolve_all(self, author: str | None, resolve: Callable[[int, dict[str, lis itself is still linear in the number of revisions listed. """ count = 0 - element_index = self._revision_element_index() + element_index = self._revision_element_index(author) with self._deferred_range_sweep(): while True: revisions = self.list_revisions(author=author, with_location=False) @@ -345,9 +357,9 @@ def accept_all(self, author: str | None = None) -> ResolveResult: ``_resolve_all``, which re-lists on each pass: that fully resolves *nested* revisions in Word-authored files (e.g. a w:del inside a w:ins) and terminates even when an author filter leaves other authors' - revisions in the document. Revisions are matched by w:id, so if Word - emits duplicate ids across authors, a filtered call may also process a - same-id revision by another author. + revisions in the document. An ``author=`` call resolves only that + author's own elements even when a producer repeats a w:id across + authors: the id lookup is scoped to the author being resolved. A move is resolved as a unit: its ``w:moveFrom`` content is removed and its ``w:moveTo`` content unwrapped in the same call, and the range @@ -395,9 +407,9 @@ def reject_all(self, author: str | None = None) -> ResolveResult: ``_resolve_all``, which re-lists on each pass: that fully resolves *nested* revisions in Word-authored files (e.g. a w:del inside a w:ins) and terminates even when an author filter leaves other authors' - revisions in the document. Revisions are matched by w:id, so if Word - emits duplicate ids across authors, a filtered call may also process a - same-id revision by another author. + revisions in the document. An ``author=`` call resolves only that + author's own elements even when a producer repeats a w:id across + authors: the id lookup is scoped to the author being resolved. A move is undone as a unit: its ``w:moveTo`` content is removed and its ``w:moveFrom`` content restored in place, range marks swept — the From 489e44da74322ccabb3eb85a3f67a1b8f812ee49 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sun, 30 Aug 2026 23:44:49 +0200 Subject: [PATCH 02/14] test: pin author-filtered resolution against duplicate w:ids across authors (ROADMAP.md #75) Flips the pin that documented the defect (accept_all's filtered pass no longer resolves another author's same-id revision as collateral) and adds the exact ROADMAP shape -- B's w:ins w:id="7" before A's w:del w:id="7" -- with the data-loss case, the accept counterpart, both unfiltered paths (so the scoping cannot regress into over-filtering), and a count-equals-listed-rows check. test_unattributed_revision_resolves_under_the_unknown_author covers the other half of the index's author expression: dropping the "Unknown" fallback leaves accept_all(author="Unknown") returning 0 on an unattributed revision, with the revision still pending and no warning raised, and passed every other test. TestGroupResolutionWithDuplicateIds pins why the group/changeset path needs no author scoping: both duplicate rows report group_id and changeset_id None, the neighbouring real group resolves exactly its own member, and the duplicated id is not a group or changeset id either. --- tests/test_revision_groups.py | 63 +++++++++++++++ tests/test_track_changes.py | 146 +++++++++++++++++++++++++++++++--- 2 files changed, 198 insertions(+), 11 deletions(-) diff --git a/tests/test_revision_groups.py b/tests/test_revision_groups.py index e1f2697..500318b 100644 --- a/tests/test_revision_groups.py +++ b/tests/test_revision_groups.py @@ -1687,6 +1687,69 @@ def test_foreign_revisions_carry_inferred_changesets(self, test_data_dir, temp_d assert all(len(keys) == 1 for keys in by_cs.values()) +class TestGroupResolutionWithDuplicateIds: + """Why group/changeset resolution needs no author scoping (ROADMAP.md #75). + + ``accept_all(author=...)`` scopes its w:id -> element index to the author, + because a whole-document pass can meet the same id twice. ``_resolve_ids`` + (the group/changeset path) does not, and these pin the two reasons: a + duplicated id is never a group or changeset member (``_reconstruct_groups`` + bars every occurrence), and our own allocator cannot mint a colliding id + (``_seed_max_change_id`` folds every id in the file into a high-water mark). + """ + + # B's insertion and A's deletion share w:id="7"; id 9 is a normal, + # single-author revision alongside them. + DUPLICATE_ID_BODY = ( + f'BEE' + f'' + "AYE" + f"{_ins_xml(9, 'solo')}" + ) + + def test_duplicated_id_is_never_a_group_or_changeset_member(self, temp_xml): + manager = _make_manager(temp_xml(self.DUPLICATE_ID_BODY)) + + duplicates = [rev for rev in manager.list_revisions() if rev.id == 7] + assert len(duplicates) == 2 + assert {rev.author for rev in duplicates} == {AUTHOR_A, AUTHOR_B} + for rev in duplicates: + assert rev.group_id is None and rev.changeset_id is None + # The neighbouring single-author revision groups normally. + (solo,) = [rev for rev in manager.list_revisions() if rev.id == 9] + assert solo.group_id is not None and solo.changeset_id is not None + + @pytest.mark.parametrize("method", ["accept_group", "reject_group"]) + def test_group_resolution_leaves_both_duplicate_occurrences_pending(self, temp_xml, method): + manager = _make_manager(temp_xml(self.DUPLICATE_ID_BODY)) + (solo,) = [rev for rev in manager.list_revisions() if rev.id == 9] + assert solo.group_id is not None + + assert getattr(manager, method)(solo.group_id) == 1 + + # Only the group's own member resolved; neither id-7 element was a + # candidate, so no author check was needed to spare them. + assert sorted(rev.id for rev in manager.list_revisions()) == [7, 7] + + @pytest.mark.parametrize("method", ["accept_changeset", "reject_changeset"]) + def test_changeset_resolution_leaves_both_duplicate_occurrences_pending(self, temp_xml, method): + manager = _make_manager(temp_xml(self.DUPLICATE_ID_BODY)) + (solo,) = [rev for rev in manager.list_revisions() if rev.id == 9] + assert solo.changeset_id is not None + + assert getattr(manager, method)(solo.changeset_id) == 1 + + assert sorted(rev.id for rev in manager.list_revisions()) == [7, 7] + + @pytest.mark.parametrize("method", ["accept_group", "reject_group", "accept_changeset", "reject_changeset"]) + def test_unknown_id_still_raises(self, temp_xml, method): + manager = _make_manager(temp_xml(self.DUPLICATE_ID_BODY)) + + # A duplicated w:id is not a group or changeset id either. + with pytest.raises(RevisionError): + getattr(manager, method)(7) + + class TestAcceptPathIndex: """The group/changeset accept path builds one w:id->element index per call instead of scanning the whole document per member (ISSUES.md #57). diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index 73945a9..8a493af 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1066,6 +1066,17 @@ class TestNestedForeignRevisions: """ + # The ROADMAP.md #75 shape: B's insertion carries the same w:id as A's + # deletion and comes *first* in document order, so an unscoped id lookup + # hands B's element to every one of A's passes. + DUPLICATE_ID_ACROSS_AUTHORS = """ + + BEE + + + AYE + """ + def test_accept_all_stops_when_no_revision_can_be_resolved(self): """The no-progress guard in _resolve_all: listed but unresolvable stops. @@ -1140,11 +1151,11 @@ def test_reject_all_nested_inner_processed_first(self): assert dom.getElementsByTagName("w:t") == [] assert dom.getElementsByTagName("w:delText") == [] - def test_accept_all_author_filter_duplicate_ids_converges(self): - """Test that accept_all(author=...) converges when w:id values collide across authors.""" - # Word does not guarantee unique w:id across w:ins/w:del. The per-id - # lookup checks w:ins first, so accepting B's deletion by id hits A's - # insertion instead; only re-listing until no progress resolves B. + def test_accept_all_author_filter_leaves_other_authors_same_id_revision(self): + """Test that accept_all(author=...) skips another author's revision sharing its w:id.""" + # Word does not guarantee unique w:id across w:ins/w:del. Resolution is + # still keyed on the id, but the id lookup is scoped to the author being + # resolved, so A's same-id insertion is not a candidate for B's pass. body = """ alpha @@ -1157,13 +1168,17 @@ def test_accept_all_author_filter_duplicate_ids_converges(self): count = manager.accept_all(author="B") - # Documented side effect of id-based matching: pass 1 accepts A's - # same-id insertion (collateral), pass 2 accepts B's deletion. - assert count == 2 - assert manager.list_revisions() == [] - texts = [t.firstChild.data for t in dom.getElementsByTagName("w:t")] - assert texts == ["alpha"] + # Only B's deletion: the count equals the number of B's listed rows. + assert count == 1 + assert manager.list_revisions(author="B") == [] assert dom.getElementsByTagName("w:delText") == [] + # A's insertion is untouched — still pending, still wrapped in w:ins. + remaining = manager.list_revisions(author="A") + assert len(remaining) == 1 + assert remaining[0].type == "insertion" + ins = dom.getElementsByTagName("w:ins") + assert len(ins) == 1 + assert [t.firstChild.data for t in ins[0].getElementsByTagName("w:t")] == ["alpha"] def test_accept_all_author_filter_terminates_with_foreign_revisions(self): """Test that accept_all(author=...) terminates while other authors' revisions remain.""" @@ -1193,6 +1208,115 @@ def test_reject_all_author_filter_terminates_with_foreign_revisions(self): texts = [t.firstChild.data for t in dom.getElementsByTagName("w:t")] assert texts == ["kept", "gone"] + def test_reject_all_author_filter_spares_other_authors_same_id_insertion(self): + """Test that reject_all(author=...) does not destroy another author's same-id insertion. + + The data-loss case of ROADMAP.md #75: rejecting A's deletion once + reached B's earlier same-id insertion and removed its text outright. + """ + manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS) + dom = manager.editor.dom + + count = manager.reject_all(author="A") + + assert count == 1 + assert manager.list_revisions(author="A") == [] + # A's rejected deletion put its text back as plain text. + assert dom.getElementsByTagName("w:delText") == [] + # B's insertion survives, still pending and still wrapped in its w:ins. + remaining = manager.list_revisions(author="B") + assert len(remaining) == 1 + assert remaining[0].type == "insertion" + assert remaining[0].id == 7 + ins = dom.getElementsByTagName("w:ins") + assert len(ins) == 1 + assert ins[0].getAttribute("w:author") == "B" + assert [t.firstChild.data for t in ins[0].getElementsByTagName("w:t")] == ["BEE"] + assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["BEE", "AYE"] + + def test_accept_all_author_filter_leaves_other_authors_same_id_insertion_pending(self): + """Test that accept_all(author=...) does not make another author's same-id insertion permanent.""" + manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS) + dom = manager.editor.dom + + count = manager.accept_all(author="A") + + assert count == 1 + assert manager.list_revisions(author="A") == [] + # A's deletion applied: its text is gone. + assert dom.getElementsByTagName("w:delText") == [] + # B's insertion is still an unadjudicated insertion, not plain text. + remaining = manager.list_revisions(author="B") + assert len(remaining) == 1 + assert remaining[0].type == "insertion" + ins = dom.getElementsByTagName("w:ins") + assert len(ins) == 1 + assert ins[0].getAttribute("w:author") == "B" + assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["BEE"] + + def test_reject_all_unfiltered_still_resolves_both_same_id_revisions(self): + """Test that reject_all() with no author still resolves every same-id revision in one call.""" + manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS) + dom = manager.editor.dom + + count = manager.reject_all() + + # The author scoping must not narrow the unfiltered path: both + # duplicate-id revisions still resolve. + assert count == 2 + assert manager.list_revisions() == [] + assert dom.getElementsByTagName("w:ins") == [] + assert dom.getElementsByTagName("w:delText") == [] + assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["AYE"] + + def test_accept_all_unfiltered_still_resolves_both_same_id_revisions(self): + """Test that accept_all() with no author still resolves every same-id revision in one call.""" + manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS) + dom = manager.editor.dom + + count = manager.accept_all() + + assert count == 2 + assert manager.list_revisions() == [] + assert dom.getElementsByTagName("w:ins") == [] + assert dom.getElementsByTagName("w:delText") == [] + assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["BEE"] + + def test_accept_all_author_filter_resolves_the_rows_it_listed(self): + """Test that a filtered call's count equals the number of that author's listed rows. + + Index and listing must agree on ownership: if the index used a + different author expression, the filtered pass would list rows it + could not resolve and exit on the no-progress guard — a silent no-op. + """ + manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS) + + listed = len(manager.list_revisions(author="B")) + + assert manager.accept_all(author="B") == listed == 1 + + @pytest.mark.parametrize("method", ["accept_all", "reject_all"]) + def test_unattributed_revision_resolves_under_the_unknown_author(self, method): + """Test that author="Unknown" resolves a revision whose w:author is absent. + + The index and the listing must read a missing ``w:author`` the same + way. ``list_revisions`` reports such a mark as ``"Unknown"``, so an + index keyed on the raw attribute would list a row it could not + resolve: the pass would exit on the no-progress guard, returning 0 + with the revision still pending and no warning raised. + """ + manager = _make_revision_manager( + """ + BEE + AYE""" + ) + + assert getattr(manager, method)(author="Unknown") == 1 + + assert manager.list_revisions(author="Unknown") == [] + # A's attributed revision is untouched. + assert [rev.id for rev in manager.list_revisions()] == [8] + class TestRestoreDeletionAttributeCopying: """Tests for _restore_deletion attribute copying edge cases.""" From dc8d695c95898bb588e95be8f3c566d3f04deb3f Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sun, 30 Aug 2026 23:44:49 +0200 Subject: [PATCH 03/14] docs: state that author= resolution holds across duplicate w:ids (ROADMAP.md #75) The "author= filters process each author's changes independently" bullet now says the guarantee survives a producer repeating a w:id across authors. --- skills/docx/SKILL.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index cd499a4..cceacf7 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -674,8 +674,10 @@ nested deletion's `nested_under` points back at its host. disappear with it. - `accept_all()` / `reject_all()` resolve nesting fully (they re-scan until no listed revision remains), and `author=` filters process each author's - changes independently. Revision types outside `list_revisions()` are not - resolved at all — see the `result.unhandled` recipe above. + changes independently — including when a producer repeats a `w:id` across + authors, where a filtered call still resolves only that author's own + elements. Revision types outside `list_revisions()` are not resolved at + all — see the `result.unhandled` recipe above. **Moves are two rows.** A drag-and-drop move lists as a `move_from` (the source, text excluded from the visible view) and a `move_to` (the From 387a37a090ba84f2b8a23cb2ea7c72a8752995b6 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 00:35:25 +0200 Subject: [PATCH 04/14] fix: key the w:id index on the id the listing reports (ROADMAP.md #75) The index keyed on the raw w:id attribute while every lookup asks for str(int(w:id)) and the listing reports int(w:id), so a nonconforming w:id="007" listed as id 7 and was reachable by nothing: accept_all() returned 0, left it pending, and reported a clean document. Nothing warned, because a mark with a numeric id is not part of the unhandled honesty floor -- and groups_are_dead() read the live group as dead, which reaps its note comment. Both sides now normalize on _adjudicable_id: the index keys on str of it and drops elements that have none (nothing id-keyed can reach those, and _unhandled_elements already reports them), and the fresh scan in _find_revision_element compares the parsed id instead of the raw string. Every consumer already looked ids up in the normalized form. _revision_author joins _adjudicable_id in dom.py as the one definition of the other half of a revision's identity, replacing five copies of `getAttribute("w:author") or "Unknown"` in listing.py. That makes the index-and-listing agreement this fix rests on structural rather than a docstring claim backed by one regression test. --- docx_editor/track_changes/dom.py | 12 ++++++++ docx_editor/track_changes/listing.py | 34 +++++++++++++++------- tests/test_track_changes.py | 43 ++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 11 deletions(-) diff --git a/docx_editor/track_changes/dom.py b/docx_editor/track_changes/dom.py index 1eca93a..14f058c 100644 --- a/docx_editor/track_changes/dom.py +++ b/docx_editor/track_changes/dom.py @@ -71,6 +71,18 @@ def _adjudicable_id(elem) -> int | None: return None +def _revision_author(elem) -> str: + """``elem``'s ``w:author``, or ``"Unknown"`` when the attribute is absent. + + The companion to ``_adjudicable_id`` for the other half of a revision's + identity, and shared for the same reason: the listing, the honesty floor + and the w:id index all filter on author, and an author-filtered + ``accept_all``/``reject_all`` resolves nothing at all if the index reads an + unattributed mark differently from the listing that selected it. + """ + return elem.getAttribute("w:author") or "Unknown" + + def _nearest_revision_ancestor_id(elem) -> int | None: """id of the closest handled-revision ancestor with a numeric w:id, else None. diff --git a/docx_editor/track_changes/listing.py b/docx_editor/track_changes/listing.py index 1d83da8..765e5e7 100644 --- a/docx_editor/track_changes/listing.py +++ b/docx_editor/track_changes/listing.py @@ -22,6 +22,7 @@ _nearest_revision_ancestor_id, _occurrence_in_text_map, _parse_w_date, + _revision_author, ) from .models import ( _MARKUP_KIND_BY_TAG, @@ -183,7 +184,7 @@ def render(node) -> str: if child.tagName in _MARKUP_KIND_BY_TAG: kind = _MARKUP_KIND_BY_TAG[child.tagName] rev_id = child.getAttribute("w:id") or "?" - rev_author = child.getAttribute("w:author") or "Unknown" + rev_author = _revision_author(child) parts.append(f"[{kind}#{rev_id}:{rev_author}]{render(child)}[/{kind}]") elif child.tagName in ("w:t", "w:delText"): parts.append(get_text_node_data(child)) @@ -219,7 +220,7 @@ def _parse_revision( # ``list_unhandled_revisions`` instead (see ``_unhandled_elements``). return None - author = elem.getAttribute("w:author") or "Unknown" + author = _revision_author(elem) date = _parse_w_date(elem) # Extract text content @@ -300,9 +301,18 @@ def _revision_element_index(self, author: str | None = None) -> dict[str, list[E ``author`` scopes the index to that author's elements, which is what makes an author-filtered ``_resolve_all`` unable to reach another author's same-id revision: the lookup has no other candidate to - return. The author test is ``list_revisions``'/``_unhandled_elements``' - — a missing ``w:author`` reads as ``"Unknown"`` — so the index and the - listing cannot disagree about who owns an element. + return. Ownership is ``_revision_author`` — the same test + ``list_revisions`` and ``_unhandled_elements`` apply, so the index and + the listing cannot disagree about who owns an element. + + Keys are ``_adjudicable_id`` in string form, not the raw attribute, + for the matching reason on the other half of a revision's identity: + the listing reports ``int(w:id)`` and every lookup asks for + ``str(int)``, so a raw key would strand a nonconforming ``w:id="007"`` + — listed as id 7, unreachable through the index, and silently counted + as resolved-nothing by a call that reported the document clean. An + element with no numeric id is dropped rather than keyed: nothing + id-based can reach it, and ``_unhandled_elements`` is what reports it. Each id's list is in document order — the same order the fresh scan in ``_find_revision_element`` uses, so a duplicated id resolves the @@ -310,9 +320,12 @@ def _revision_element_index(self, author: str | None = None) -> dict[str, list[E """ element_index: dict[str, list[Element]] = {} for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): - if author is not None and (elem.getAttribute("w:author") or "Unknown") != author: + rev_id = _adjudicable_id(elem) + if rev_id is None: continue - element_index.setdefault(elem.getAttribute("w:id"), []).append(elem) + if author is not None and _revision_author(elem) != author: + continue + element_index.setdefault(str(rev_id), []).append(elem) return element_index def _is_in_document(self, elem) -> bool: @@ -352,9 +365,8 @@ def _find_revision_element( filtered call cannot land on another author's same-id revision. """ if element_index is None: - wanted = str(revision_id) for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): - if elem.getAttribute("w:id") == wanted: + if _adjudicable_id(elem) == revision_id: return elem return None for elem in element_index.get(str(revision_id), ()): @@ -391,7 +403,7 @@ def _unhandled_elements(self, author: str | None = None) -> list[Element]: ] if author is None: return elems - return [e for e in elems if (e.getAttribute("w:author") or "Unknown") == author] + return [e for e in elems if _revision_author(e) == author] def list_unhandled_revisions(self, author: str | None = None) -> list[UnhandledRevision]: """List the revision elements this library does not accept or reject. @@ -433,7 +445,7 @@ def list_unhandled_revisions(self, author: str | None = None) -> list[UnhandledR UnhandledRevision( tag=elem.tagName, id=_adjudicable_id(elem), - author=elem.getAttribute("w:author") or "Unknown", + author=_revision_author(elem), date=_parse_w_date(elem), paragraph_ref=ctx.paragraph_ref(paragraph) if paragraph is not None else None, ) diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index 8a493af..799c025 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1317,6 +1317,49 @@ def test_unattributed_revision_resolves_under_the_unknown_author(self, method): # A's attributed revision is untouched. assert [rev.id for rev in manager.list_revisions()] == [8] + @pytest.mark.parametrize("method", ["accept_all", "reject_all"]) + def test_non_canonical_id_resolves_through_the_index(self, method): + """Test that a w:id whose raw form is not str(int) still resolves. + + The id half of the same index/listing agreement: the listing reports + ``int(w:id)`` and every lookup asks for ``str(int)``, so an index keyed + on the raw attribute strands ``w:id="007"`` — listed as id 7, resolved + by nothing, and reported as a clean document by a call that left it + pending. Nothing warns, because the mark has a numeric id and so is not + part of the unhandled honesty floor. + """ + manager = _make_revision_manager('zero') + + assert [rev.id for rev in manager.list_revisions()] == [7] + assert getattr(manager, method)() == 1 + assert manager.list_revisions() == [] + assert manager.list_unhandled_revisions() == [] + + def test_non_canonical_id_resolves_without_an_index(self): + """Test that the fresh-scan path matches a non-canonical w:id too. + + ``accept_revision`` with no pre-built index scans the document itself; + it must read the id the same way the index does, or a standalone call + would fail on the ids a bulk call resolves. + """ + manager = _make_revision_manager('zero') + + assert manager.accept_revision(7) is True + assert manager.list_revisions() == [] + + def test_non_numeric_id_is_left_to_the_honesty_floor(self): + """Test that a mark with no numeric w:id is not indexed and is reported. + + ``_adjudicable_id`` is None for it, so nothing id-keyed can reach it: + it must stay out of the index (an unreachable key) and out of + ``list_revisions``, and surface in ``list_unhandled_revisions``. + """ + manager = _make_revision_manager('x') + + assert manager._revision_element_index() == {} + assert manager.list_revisions() == [] + assert [u.tag for u in manager.list_unhandled_revisions()] == ["w:ins"] + class TestRestoreDeletionAttributeCopying: """Tests for _restore_deletion attribute copying edge cases.""" From f800e3d51c3a0499f880ed4504df5c21da68a5c5 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 00:35:25 +0200 Subject: [PATCH 05/14] test: ask for an unallocated group id rather than a revision id (ROADMAP.md #75) test_unknown_id_still_raises passed for the wrong reason: it asked accept_group/accept_changeset for id 7 -- a w:id -- and group and changeset ids come from their own counters, so it raised only because this fixture's counters never reach 7, and would have started testing the opposite thing had they. It now asks for an id past the one actually allocated. DUPLICATE_ID_BODY builds its runs with the file's own _ins_xml/_del_xml helpers, which emit byte-identical XML, instead of hand-writing them. --- tests/test_revision_groups.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/test_revision_groups.py b/tests/test_revision_groups.py index 500318b..22e49de 100644 --- a/tests/test_revision_groups.py +++ b/tests/test_revision_groups.py @@ -1701,9 +1701,7 @@ class TestGroupResolutionWithDuplicateIds: # B's insertion and A's deletion share w:id="7"; id 9 is a normal, # single-author revision alongside them. DUPLICATE_ID_BODY = ( - f'BEE' - f'' - "AYE" + f"{_ins_xml(7, 'BEE', author=AUTHOR_B, date=DATE_B)}{_del_xml(7, 'AYE')}" f"{_ins_xml(9, 'solo')}" ) @@ -1742,12 +1740,18 @@ def test_changeset_resolution_leaves_both_duplicate_occurrences_pending(self, te assert sorted(rev.id for rev in manager.list_revisions()) == [7, 7] @pytest.mark.parametrize("method", ["accept_group", "reject_group", "accept_changeset", "reject_changeset"]) - def test_unknown_id_still_raises(self, temp_xml, method): + def test_unknown_group_or_changeset_id_still_raises(self, temp_xml, method): manager = _make_manager(temp_xml(self.DUPLICATE_ID_BODY)) - # A duplicated w:id is not a group or changeset id either. + # Group and changeset ids come from their own counters, a namespace + # apart from w:id -- so this asks for one that was never allocated, + # not for the duplicated revision id (which would raise for the + # unrelated reason that this fixture's counters never reach it). + (solo,) = [rev for rev in manager.list_revisions() if rev.id == 9] + allocated = solo.group_id if "group" in method else solo.changeset_id + assert allocated is not None with pytest.raises(RevisionError): - getattr(manager, method)(7) + getattr(manager, method)(allocated + 1000) class TestAcceptPathIndex: From 21a49968c96450fd07efdd25cb547baada9993bb Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 00:35:25 +0200 Subject: [PATCH 06/14] docs: note the per-id duplicate-w:id caveat and file the adjacent defects The duplicate-w:id guarantee belongs to accept_all/reject_all alone: accept_revision(id)/reject_revision(id) take only the id, so a duplicated one resolves whichever element comes first in the document. SKILL.md recommended the per-id pattern with no caveat; it now points at the bulk call or the row's group_id when an id is shared. Files three defects the PR #96 multi-review turned up, all pre-existing and each needing its own change and regression test: - #83 the per-id half of #75, with a zero-cost ambiguity test the manager already computes - #84 _rejoin_paragraph drops the tail paragraph's w:pPr, destroying another author's paragraph-mark revisions uncounted - #85 marks recorded inside a change record list as live revisions --- ROADMAP.md | 12 ++++++++++++ skills/docx/SKILL.md | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/ROADMAP.md b/ROADMAP.md index f9e38af..aec569f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -73,6 +73,18 @@ Only `w:instrText` is known to the library today. A `replace()` targeting text i The #91 diagnostics caught it on their first CI failure (run 33331432527, load average 1.14 — an idle runner): the kernel wrote `kernel.json` in 0.5 s, then our `kernel_info` probe on the control channel raised inside ipykernel's ZMQStream callback — `KeyError: '_control_lock'` (traitlets) — because the control message arrived before the kernel finished initializing; the callback's exception left the control stream broken and the kernel never replied within the remaining 59.5 s. So the residual "flake" is a race our own probe triggers, not load. Fix directions, in preference order: probe on the shell channel or wait for the iopub `status: starting` message before touching control; retry with a *fresh* client (the broken stream may be permanent for that connection); check whether current ipykernel has fixed `_control_lock` initialization order and pin/upgrade. Regression test: a kernel whose control channel receives `kernel_info` immediately after `kernel.json` appears must still become ready. +### 83. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss] + +The per-id half of #75, which fixed only `accept_all`/`reject_all`. With B's `` before A's ``, `list_revisions(author="A")` returns one row with id 7, and `reject_revision(7)` deletes B's inserted text and leaves A's deletion pending; `accept_revision(7)` makes B's insertion permanent. The caller cannot disambiguate — `Revision` carries no element handle — and `SKILL.md` documents this exact per-id pattern. Fix: either give the per-id verbs an author/identity selector, or refuse an ambiguous id with a teaching error; the manager already knows which ids are ambiguous, since `_reconstruct_groups` is the sole writer of `self._revision_groups[rev_id] = None` (`registry.py`) and writes it only for duplicated ids. SKILL.md carries an interim caveat. Found by the multi-review of PR #96. + +### 84. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss] + +Rejecting a paragraph-mark insertion migrates only the mark `w:ins` off the tail paragraph and then drops that paragraph's whole `w:pPr`, so another author's paragraph-mark `w:del`/`w:moveFrom`/`w:moveTo` and their `w:pPrChange` are destroyed — uncounted, unwarned, and with the merged paragraph's break left permanently untracked. Reproduces with no author filter, and is order-dependent within a single author: the same document returns 1 or 2 depending only on whether the mark-ins id sorts above the mark-del id. The `accept_all` docstring names only the `w:pPrChange` case. Fix: migrate the whole downstream mark set onto the surviving paragraph's mark `rPr`, since they all describe the break it now owns. Found by the multi-review of PR #96. + +### 85. Historical marks inside change records list as live revisions + +`list_revisions` and `_revision_element_index` walk with `skip_change_records=False` while `_unhandled_elements` uses `True`, so a revision mark recorded *inside* a change record is offered as adjudicable. `w:pPr/w:rPr/w:rPrChange/w:rPr/w:ins` is conforming (CT_ParaRPrOriginal admits EG_ParaRPrTrackChanges) and lists as a live insertion, and resolving it mutates the other author's change record; the nonconforming `w:pPrChange`-holding-`w:rPr` shape is worse — it merges two live paragraphs. Fix: have both walks pass `skip_change_records=True` and leave recorded marks to the honesty floor. Needs its own regression test. Found by the multi-review of PR #96. + ### 69. Return-leg reconciliation: which changesets survived? [design] The product loop is agent proposes → human adjudicates in Word → document comes back; accepted revisions become plain text, rejected ones vanish, and there is no way to ask "which of my changesets survived?". A sent-vs-returned compare keyed on changeset `w:date` + content. Design first; large. diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index cceacf7..68afc05 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -678,6 +678,12 @@ nested deletion's `nested_under` points back at its host. authors, where a filtered call still resolves only that author's own elements. Revision types outside `list_revisions()` are not resolved at all — see the `result.unhandled` recipe above. +- That duplicate-`w:id` guarantee is the bulk calls' alone. A producer may + reuse one `w:id` across authors, and `accept_revision(id)`/ + `reject_revision(id)` take only the id: handed a duplicated one they resolve + whichever element comes first in the document, which may be the other + author's. When a listed revision shares its id with a second row, prefer + `accept_all(author=…)` / the row's `group_id`. **Moves are two rows.** A drag-and-drop move lists as a `move_from` (the source, text excluded from the visible view) and a `move_to` (the From 4f8ec9689dc30363a655616fdaa86c6bd0307b52 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 00:53:48 +0200 Subject: [PATCH 07/14] fix: resolving revision id None must match nothing (ROADMAP.md #75) Keying the fresh scan on _adjudicable_id made None match: the parse returns None for exactly the marks no id-keyed call can reach, so an unguarded comparison handed back the first of them. list_unhandled_revisions() reports id=None for such a row, so this was reachable from the library's own output -- reject_revision(None) returned True and destroyed the mark's text, where before it returned False. The index path never matched (no "None" key), so the two lookup paths disagreed as well. get_markup_text renders the adjudicable id too, so the one view that names revision ids in text names the id accept_revision actually takes ("?" when there is none) rather than the raw attribute. Tests: id None resolves nothing through either verb and leaves the mark and its text in place; and author scoping still holds for ids that collide only once parsed (B's w:id="007" against A's w:id="7"), the duplicate-id class that normalizing the key widened. --- docx_editor/track_changes/listing.py | 23 ++++++++++-- tests/test_track_changes.py | 55 ++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/docx_editor/track_changes/listing.py b/docx_editor/track_changes/listing.py index 765e5e7..b746099 100644 --- a/docx_editor/track_changes/listing.py +++ b/docx_editor/track_changes/listing.py @@ -71,9 +71,13 @@ def has_own_revisions(self) -> bool: Counts *pending* revisions only — accepting or rejecting one removes it from the DOM, so a document we edited and then fully accepted answers False, which is the honest answer: it carries no redline. - Ownership is the same test the rest of this class uses, ``w:author`` - equal to the editor's author, so a foreign redline by someone whose - author string happens to match ours reads as ours. + Ownership is ``w:author`` equal to the editor's author, so a foreign + redline by someone whose author string happens to match ours reads as + ours. Deliberately the raw attribute, not ``_revision_author``: the + listing's ``"Unknown"`` fallback exists so an unattributed mark can be + *named* in a filter, and folding it in here would hand every + unattributed mark in the file to an editor who happens to be called + "Unknown". Returns: True on the first ``w:ins``/``w:del`` we authored, False if there @@ -183,7 +187,11 @@ def render(node) -> str: continue if child.tagName in _MARKUP_KIND_BY_TAG: kind = _MARKUP_KIND_BY_TAG[child.tagName] - rev_id = child.getAttribute("w:id") or "?" + # The adjudicable id, not the raw attribute: this view + # names the id ``accept_revision`` takes, and "?" marks + # one it cannot take at all. + parsed_id = _adjudicable_id(child) + rev_id = "?" if parsed_id is None else str(parsed_id) rev_author = _revision_author(child) parts.append(f"[{kind}#{rev_id}:{rev_author}]{render(child)}[/{kind}]") elif child.tagName in ("w:t", "w:delText"): @@ -365,6 +373,13 @@ def _find_revision_element( filtered call cannot land on another author's same-id revision. """ if element_index is None: + if revision_id is None: + # _adjudicable_id is None for exactly the marks no id-keyed + # call can reach, so an unguarded == would match the first of + # them — and None is what list_unhandled_revisions() reports + # for such a row. The index path misses (no "None" key); this + # keeps the two paths agreeing on unreachable. + return None for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): if _adjudicable_id(elem) == revision_id: return elem diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index 799c025..aa42e41 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1295,6 +1295,31 @@ def test_accept_all_author_filter_resolves_the_rows_it_listed(self): assert manager.accept_all(author="B") == listed == 1 + def test_author_filter_holds_when_ids_collide_only_after_normalizing(self): + """Test that author scoping holds for ids that are equal only once parsed. + + ``w:id="007"`` and ``w:id="7"`` are distinct raw attributes that share + one index key, so normalizing the key widened the duplicate-id class + this fix guards. The author scoping has to cover the widened class too. + """ + manager = _make_revision_manager( + """ + BEE + AYE""" + ) + dom = manager.editor.dom + + assert manager.reject_all(author="A") == 1 + + # B's insertion is untouched: still pending, still wrapped. + remaining = manager.list_revisions(author="B") + assert len(remaining) == 1 + assert remaining[0].type == "insertion" + ins = dom.getElementsByTagName("w:ins") + assert len(ins) == 1 + assert ins[0].getAttribute("w:author") == "B" + assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["BEE", "AYE"] + @pytest.mark.parametrize("method", ["accept_all", "reject_all"]) def test_unattributed_revision_resolves_under_the_unknown_author(self, method): """Test that author="Unknown" resolves a revision whose w:author is absent. @@ -1317,6 +1342,15 @@ def test_unattributed_revision_resolves_under_the_unknown_author(self, method): # A's attributed revision is untouched. assert [rev.id for rev in manager.list_revisions()] == [8] + +class TestRevisionIdNormalization: + """The w:id half of index/listing agreement (ROADMAP.md #75). + + ``list_revisions`` reports ``int(w:id)`` and every lookup asks for + ``str(int)``, so the index and the fresh scan must read an id the same way + or a nonconforming ``w:id`` is listed but reachable by nothing. + """ + @pytest.mark.parametrize("method", ["accept_all", "reject_all"]) def test_non_canonical_id_resolves_through_the_index(self, method): """Test that a w:id whose raw form is not str(int) still resolves. @@ -1360,6 +1394,27 @@ def test_non_numeric_id_is_left_to_the_honesty_floor(self): assert manager.list_revisions() == [] assert [u.tag for u in manager.list_unhandled_revisions()] == ["w:ins"] + @pytest.mark.parametrize("method", ["accept_revision", "reject_revision"]) + def test_none_id_resolves_nothing(self, method): + """Test that resolving id None is a no-op, not a match on the first unreachable mark. + + ``list_unhandled_revisions()`` reports ``id=None`` for a mark with no + numeric ``w:id``, so None is a value callers really do hold. The fresh + scan compares parsed ids, and ``_adjudicable_id`` is None for exactly + those marks — unguarded, None would match the first of them and + resolve what the honesty floor just called unresolvable. + """ + manager = _make_revision_manager('KEEP') + (row,) = manager.list_unhandled_revisions() + assert row.id is None + + assert getattr(manager, method)(row.id) is False + + # The mark and its text are untouched. + assert [u.tag for u in manager.list_unhandled_revisions()] == ["w:ins"] + texts = [t.firstChild.data for t in manager.editor.dom.getElementsByTagName("w:t")] + assert texts == ["KEEP"] + class TestRestoreDeletionAttributeCopying: """Tests for _restore_deletion attribute copying edge cases.""" From 46aad5ba0e1009ce7eac7f73882e7b08615bc04a Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 00:53:48 +0200 Subject: [PATCH 08/14] docs: correct the duplicate-w:id guidance and sync docs/api.md The per-id caveat advised falling back to the row's group_id, which is exactly what a duplicated id does not have: _reconstruct_groups bars every occurrence from grouping, as this PR's own TestGroupResolutionWithDuplicateIds asserts. Two rows sharing an id in list_revisions() is the signal instead, and the author-filtered bulk calls are the way to resolve them. docs/api.md carried neither the author-scoping guarantee nor the per-id caveat; its accept_all/reject_all/accept_revision/reject_revision sections now match SKILL.md, as every prior change to this prose has kept them. ROADMAP #75 no longer describes the fixed behaviour as current, and records why the w:id index was scoped by author instead of the filed element-identity suggestion. #83's fix direction named _reconstruct_groups as the sole writer of an explicit None group; _adopt_split_tail also writes one for an unambiguous split tail, so it points at the duplicate_ids set instead. has_own_revisions says why it keeps the raw w:author rather than _revision_author: an editor named "Unknown" must not claim unattributed marks. --- ROADMAP.md | 4 ++-- docs/api.md | 19 +++++++++++++++++-- skills/docx/SKILL.md | 6 ++++-- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index aec569f..f4ff27d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -59,7 +59,7 @@ Only `w:instrText` is known to the library today. A `replace()` targeting text i ### 75. Author-filtered bulk resolution must not touch other authors' revisions [data loss; after #73 step 9] -`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `` before A's `` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documents this and `tests/test_track_changes.py::…:1143` pins it as expected — it is not acceptable for a data-loss path. Fix: resolve the exact elements selected by the listing (element identity), not their ids; flip the pinning test. Lives in the resolution cluster, so it lands after #73 moves it (found by CodeRabbit on PR #75; verified 2026-08-30). +`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `` before A's `` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documented this and a test pinned it as expected — not acceptable for a data-loss path (found by CodeRabbit on PR #75; verified 2026-08-30). **Fixed in PR #96**, by scoping the w:id index to the author being resolved rather than the filed suggestion of resolving element identities: `Revision` carries no element handle, so identity would have meant a wider signature change for the same outcome — a same-id element of the *same* author still resolves in one pass either way. The pinning test is flipped. The per-id verbs keep the defect; that is #83. ### 79. Inserted text loses formatting after an empty split segment [after #73 step 7] @@ -75,7 +75,7 @@ The #91 diagnostics caught it on their first CI failure (run 33331432527, load a ### 83. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss] -The per-id half of #75, which fixed only `accept_all`/`reject_all`. With B's `` before A's ``, `list_revisions(author="A")` returns one row with id 7, and `reject_revision(7)` deletes B's inserted text and leaves A's deletion pending; `accept_revision(7)` makes B's insertion permanent. The caller cannot disambiguate — `Revision` carries no element handle — and `SKILL.md` documents this exact per-id pattern. Fix: either give the per-id verbs an author/identity selector, or refuse an ambiguous id with a teaching error; the manager already knows which ids are ambiguous, since `_reconstruct_groups` is the sole writer of `self._revision_groups[rev_id] = None` (`registry.py`) and writes it only for duplicated ids. SKILL.md carries an interim caveat. Found by the multi-review of PR #96. +The per-id half of #75, which fixed only `accept_all`/`reject_all`. With B's `` before A's ``, `list_revisions(author="A")` returns one row with id 7, and `reject_revision(7)` deletes B's inserted text and leaves A's deletion pending; `accept_revision(7)` makes B's insertion permanent. The caller cannot disambiguate — `Revision` carries no element handle — and `SKILL.md` documents this exact per-id pattern. Fix: either give the per-id verbs an author/identity selector, or refuse an ambiguous id with a teaching error. The duplicate set is already computed — `duplicate_ids` in `_reconstruct_groups` (`registry.py`) — and then discarded; retain or recompute it rather than testing `_revision_groups[id] is None`, which `_adopt_split_tail` also writes for an unambiguous split tail whose origin was ungroupable. SKILL.md carries an interim caveat. Found by the multi-review of PR #96. ### 84. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss] diff --git a/docs/api.md b/docs/api.md index 932ee37..8c5e872 100644 --- a/docs/api.md +++ b/docs/api.md @@ -867,6 +867,14 @@ accepting the `move_to` and rejecting the `move_from` duplicates the text, the inverse loses it. A lone half in a damaged file behaves as what it structurally is: a `move_from` alone is a deletion, a `move_to` alone an insertion. +**Duplicate ids are ambiguous here.** A producer may reuse one `w:id` across +authors, and this call takes only the id: handed a duplicated one it resolves +whichever element comes first in the document, which may be another author's. +Groups are no way around it — a duplicated id is barred from grouping, so both +rows carry `group_id=None`. Two rows sharing an id in `list_revisions()` is the +signal; resolve them with `accept_all(author=…)`/`reject_all(author=…)`, which +is author-exact. + **Parameters:** - `revision_id` (int): ID of the revision to accept @@ -892,7 +900,8 @@ Reject a revision by ID. - For `property_change`: restores the paragraph's recorded previous properties. A record with none (LibreOffice writes a self-closing `w:pPrChange` for "previously no properties") clears them; the recorded style id is restored verbatim even when the document defines no such style (Word falls back to Normal for it) - Nested revisions: rejecting an insertion removes everything inside it — deletions another author nested inside it disappear with it -Resolve both halves of a move together — see `accept_revision()`. +Resolve both halves of a move together — see `accept_revision()`, whose +duplicate-`w:id` caveat applies here too. **Parameters:** @@ -1080,6 +1089,11 @@ marker is dropped without merging or splitting paragraphs. Only `word/document.xml` is inspected; headers, footers and footnotes are the container-parts epic (ROADMAP.md #30). +An `author=` call resolves that author's own elements and no one else's, even +where a producer repeats one `w:id` across authors (LibreOffice does): the id +lookup is scoped to the author being resolved, so another author's same-id +revision is never a candidate. + **Parameters:** - `author` (str, optional): If provided, only accept revisions by this author @@ -1109,7 +1123,8 @@ Reject every listed revision. Resolves insertions, deletions, content moves (both halves as a unit — the text is back at its source exactly once) and paragraph-property changes (the recorded previous properties are restored); every other revision type is left -pending and reported exactly as in `accept_all()`. +pending and reported exactly as in `accept_all()`. `author=` is author-exact in +the same way, duplicate `w:id`s across authors included. **Parameters:** diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index 68afc05..4cf5874 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -682,8 +682,10 @@ nested deletion's `nested_under` points back at its host. reuse one `w:id` across authors, and `accept_revision(id)`/ `reject_revision(id)` take only the id: handed a duplicated one they resolve whichever element comes first in the document, which may be the other - author's. When a listed revision shares its id with a second row, prefer - `accept_all(author=…)` / the row's `group_id`. + author's. Groups are no way around it — a duplicated id is barred from + grouping, so both rows carry `group_id=None`. Two rows sharing an id in + `list_revisions()` is the signal; resolve them with + `accept_all(author=…)`/`reject_all(author=…)`, which is author-exact. **Moves are two rows.** A drag-and-drop move lists as a `move_from` (the source, text excluded from the visible view) and a `move_to` (the From 8ee0998a03d26f1ae38cf61a5d1194134700962d Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 01:17:47 +0200 Subject: [PATCH 09/14] fix: read a revision id the same way on both lookup paths (ROADMAP.md #75) Keying the index on the parsed id left the two paths comparing different things: the fresh scan compared _adjudicable_id's int, the index looked up str(id). Anything int-equal but not an int then resolved through one and not the other -- accept_revision(True) destroyed revision 1 on the scan path (bool subclasses int) while .get("True") found nothing, and None, which list_unhandled_revisions() reports for a mark with no numeric id, matched every unreachable mark through == while missing the index entirely. Both sides now compare str of the adjudicable id. That restores what every id type did before this PR -- including "7", which both paths accepted and which narrowing to int would have broken for a caller round-tripping ids through JSON -- while keeping w:id="007" reachable. The None special case goes away with it, being just one value that now misses on both paths. get_markup_text's id is pinned: w:id="007" renders [ins#7:A] and a mark with no numeric id renders #?, the rows list_unhandled_revisions() reports. --- docx_editor/track_changes/base.py | 2 +- docx_editor/track_changes/listing.py | 28 ++++++++++------ tests/test_revision_location.py | 19 +++++++++++ tests/test_track_changes.py | 50 +++++++++++++++++++++++----- 4 files changed, 80 insertions(+), 19 deletions(-) diff --git a/docx_editor/track_changes/base.py b/docx_editor/track_changes/base.py index a80a56b..95e3a55 100644 --- a/docx_editor/track_changes/base.py +++ b/docx_editor/track_changes/base.py @@ -147,7 +147,7 @@ def changeset_id_of(self, group_id: int) -> int | None: raise NotImplementedError def _find_revision_element( - self, revision_id: int, element_index: dict[str, list[Element]] | None + self, revision_id: int | None, element_index: dict[str, list[Element]] | None ) -> Element | None: raise NotImplementedError diff --git a/docx_editor/track_changes/listing.py b/docx_editor/track_changes/listing.py index b746099..8827395 100644 --- a/docx_editor/track_changes/listing.py +++ b/docx_editor/track_changes/listing.py @@ -162,6 +162,9 @@ def get_markup_text(self) -> str: Each paragraph is one line; tracked changes wrap their content as ``[ins#{id}:{author}]...[/ins]`` / ``[del#{id}:{author}]...[/del]``, + where ``{id}`` is the id ``accept_revision``/``reject_revision`` take + (so a raw ``w:id="007"`` renders ``#7``) and ``#?`` marks one they + cannot reach at all — those are ``list_unhandled_revisions()`` rows — nesting included (e.g. ``[ins#1:A]kept [del#9:B]gone[/del][/ins]``). The two halves of a content move render the same way as ``[moveFrom#{id}:{author}]...[/moveFrom]`` / @@ -355,7 +358,7 @@ def _is_in_document(self, elem) -> bool: return False def _find_revision_element( - self, revision_id: int, element_index: dict[str, list[Element]] | None + self, revision_id: int | None, element_index: dict[str, list[Element]] | None ) -> Element | None: """Locate the live handled revision element for ``revision_id``. @@ -371,20 +374,25 @@ def _find_revision_element( candidates at all is the index's business: an author-scoped index (``_revision_element_index(author)``) holds only that author's, so a filtered call cannot land on another author's same-id revision. + + Both paths compare ``str`` of the *adjudicable* id, which is what keeps + them answering alike. Comparing the parsed int on one side and the + index's string key on the other would diverge on everything int-equal + but not an int: ``True`` matches id 1 through ``==`` (bool subclasses + int) but misses ``.get("True")``, and ``None`` — what + ``list_unhandled_revisions()`` reports for a mark with no numeric id — + matches every unreachable mark through ``==`` while missing the index + entirely. Stringifying both sides leaves one answer per id, whichever + path a caller reaches it by. """ + wanted = str(revision_id) if element_index is None: - if revision_id is None: - # _adjudicable_id is None for exactly the marks no id-keyed - # call can reach, so an unguarded == would match the first of - # them — and None is what list_unhandled_revisions() reports - # for such a row. The index path misses (no "None" key); this - # keeps the two paths agreeing on unreachable. - return None for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): - if _adjudicable_id(elem) == revision_id: + rev_id = _adjudicable_id(elem) + if rev_id is not None and str(rev_id) == wanted: return elem return None - for elem in element_index.get(str(revision_id), ()): + for elem in element_index.get(wanted, ()): if self._is_in_document(elem): return elem return None diff --git a/tests/test_revision_location.py b/tests/test_revision_location.py index 899202d..5275225 100644 --- a/tests/test_revision_location.py +++ b/tests/test_revision_location.py @@ -415,6 +415,25 @@ def test_markers_fall_back_on_missing_id_and_author(self, temp_xml): manager = _make_manager(temp_xml(body)) assert manager.get_markup_text() == "[ins#?:Unknown]x[/ins]" + def test_markers_render_the_id_accept_revision_takes(self, temp_xml): + """The marker names the adjudicable id, not the raw w:id attribute. + + This view exists so a reader can pick a revision and resolve it, so the + id it prints has to be the one ``accept_revision`` takes: a raw + ``w:id="007"`` is listed and resolved as 7, and a mark with no numeric + id is reachable by no id-keyed call at all — ``#?``, and reported by + ``list_unhandled_revisions()``. + """ + body = ( + 'x' + 'y' + ) + manager = _make_manager(temp_xml(body)) + + assert manager.get_markup_text() == "[ins#7:A]x[/ins][del#?:A]y[/del]" + assert [rev.id for rev in manager.list_revisions()] == [7] + assert [u.tag for u in manager.list_unhandled_revisions()] == ["w:del"] + def test_document_wrapper_shows_tracked_edit(self, temp_docx): with Document.open(temp_docx, author="Test Editor") as doc: ref = doc.list_paragraphs()[0].split("|")[0] diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index aa42e41..d3839ea 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1309,16 +1309,17 @@ def test_author_filter_holds_when_ids_collide_only_after_normalizing(self): ) dom = manager.editor.dom - assert manager.reject_all(author="A") == 1 + # Resolve B, whose id is the non-canonical one: it is reachable only + # because the index normalizes, and author-exact only because the + # index is scoped -- so this needs both halves to hold. + assert manager.reject_all(author="B") == 1 - # B's insertion is untouched: still pending, still wrapped. - remaining = manager.list_revisions(author="B") + # A's deletion is untouched: still pending, its text still deleted. + remaining = manager.list_revisions(author="A") assert len(remaining) == 1 - assert remaining[0].type == "insertion" - ins = dom.getElementsByTagName("w:ins") - assert len(ins) == 1 - assert ins[0].getAttribute("w:author") == "B" - assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["BEE", "AYE"] + assert remaining[0].type == "deletion" + assert dom.getElementsByTagName("w:ins") == [] + assert [t.firstChild.data for t in dom.getElementsByTagName("w:delText")] == ["AYE"] @pytest.mark.parametrize("method", ["accept_all", "reject_all"]) def test_unattributed_revision_resolves_under_the_unknown_author(self, method): @@ -1415,6 +1416,39 @@ def test_none_id_resolves_nothing(self, method): texts = [t.firstChild.data for t in manager.editor.dom.getElementsByTagName("w:t")] assert texts == ["KEEP"] + @pytest.mark.parametrize("bad_id", [True, 1.0, None]) + def test_int_equal_non_int_id_matches_on_neither_path(self, bad_id): + """Test that the index and the fresh scan agree on ids that are not ints. + + Both paths compare ``str`` of the adjudicable id. Comparing the parsed + int on one side and the index's string key on the other would diverge + on anything int-equal but not an int: ``True`` matches id 1 through + ``==`` (bool subclasses int) while missing ``.get("True")``, so + ``accept_revision(True)`` would resolve a revision through one path and + nothing through the other. + """ + manager = _make_revision_manager('ONE') + index = manager._revision_element_index() + + assert manager._find_revision_element(bad_id, None) is None + assert manager._find_revision_element(bad_id, index) is None + assert manager.accept_revision(bad_id) is False + assert len(manager.editor.dom.getElementsByTagName("w:ins")) == 1 + + def test_str_id_still_matches_on_both_paths(self): + """Test that a stringified id resolves, as it did before the id rework. + + Not the documented contract — ``accept_revision`` takes an int — but + both paths accepted ``"7"`` before, so narrowing to int only would + break a caller that round-trips ids through JSON or a CLI argument for + no gain. + """ + manager = _make_revision_manager('SEVEN') + index = manager._revision_element_index() + + assert manager._find_revision_element("7", None) is not None + assert manager._find_revision_element("7", index) is not None + class TestRestoreDeletionAttributeCopying: """Tests for _restore_deletion attribute copying edge cases.""" From b0600d7cce9a594fcb77f6176af05ea20db00037 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 01:17:47 +0200 Subject: [PATCH 10/14] refactor: parse revision ids through _adjudicable_id in the group registry The registry hand-rolled int(elem.getAttribute("w:id")) with a ValueError guard in four places. Identical in behaviour -- int("") raises ValueError too -- but it was the last place that answered "how is a revision id read?" on its own, after the listing, the index and the honesty floor were folded onto one definition. --- docx_editor/track_changes/registry.py | 29 +++++++++++---------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/docx_editor/track_changes/registry.py b/docx_editor/track_changes/registry.py index 854824d..f927e89 100644 --- a/docx_editor/track_changes/registry.py +++ b/docx_editor/track_changes/registry.py @@ -7,6 +7,7 @@ from ..exceptions import RevisionError from .base import _RevisionManagerBase from .dom import ( + _adjudicable_id, _ancestor_paragraph, _has_ancestor, _is_paragraph_mark_ins, @@ -106,9 +107,8 @@ def close_run() -> None: seen_ids: set[int] = set() duplicate_ids: set[int] = set() for elem in elements: - try: - elem_id = int(elem.getAttribute("w:id")) - except ValueError: + elem_id = _adjudicable_id(elem) + if elem_id is None: continue if elem_id in seen_ids: duplicate_ids.add(elem_id) @@ -118,10 +118,7 @@ def close_run() -> None: paragraph = _ancestor_paragraph(elem) author = elem.getAttribute("w:author") date = elem.getAttribute("w:date") - try: - rev_id = int(elem.getAttribute("w:id")) - except ValueError: - rev_id = None + rev_id = _adjudicable_id(elem) if rev_id is not None and _is_paragraph_mark_ins(elem): self._paragraph_mark_ids.add(rev_id) if paragraph is None or not author or not date or rev_id is None or rev_id in duplicate_ids: @@ -196,13 +193,12 @@ def _grouped(self) -> Iterator[_GroupCapture]: continue if not _has_ancestor(elem, self.editor.dom): continue - try: - # Ids we assign are always numeric; a non-numeric one was - # copied from a nonconforming producer's element — leave it - # ungrouped rather than fail the edit (same tolerance as - # _get_next_change_id). - rev_id = int(elem.getAttribute("w:id")) - except ValueError: + # Ids we assign are always numeric; a non-numeric one was copied + # from a nonconforming producer's element — leave it ungrouped + # rather than fail the edit (same tolerance as + # _get_next_change_id). + rev_id = _adjudicable_id(elem) + if rev_id is None: continue if rev_id in self._revision_groups: continue @@ -410,9 +406,8 @@ def group_spans(self, group_ids: Iterable[int]) -> dict[int, tuple[Element, Elem for elem in _revision_elements(self.editor.dom): if _is_paragraph_mark_marker(elem): continue - try: - rev_id = int(elem.getAttribute("w:id")) - except ValueError: # pragma: no cover - our own ids are always numeric + rev_id = _adjudicable_id(elem) + if rev_id is None: # pragma: no cover - our own ids are always numeric continue gid = wanted.get(rev_id) if gid is not None: From cc10ed8640a0ab7b6898b24955a96fe02ecca736 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 01:17:47 +0200 Subject: [PATCH 11/14] docs: scope the author-exact claim to targeting, and renumber the filed items docs/api.md promised an author= call "resolves that author's own elements and no one else's", which overstates a data-loss guarantee: the scoping decides which elements are targeted, not what survives, and rejecting an insertion still carries away revisions another author nested inside it. api.md's own rationale-notes section already said so. UnhandledRevision's docstring claimed its rows "carry no adjudicable id, so handing them to accept_revision() could only fail". An unhandled mark often does carry a numeric w:id, and where it collides with a handled revision's the call resolves that other element: verified, reject_revision(row.id) on a w:rPrChange id=7 deleted a w:ins id=7's text and left the rPrChange standing. Filed as #87. main added its own #83 (PR #95) after this branch forked, so the items filed here move to #84/#85/#86 and #75's cross-reference follows them. --- ROADMAP.md | 12 ++++++++---- docs/api.md | 11 +++++++---- docx_editor/track_changes/models.py | 8 ++++++-- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 69d0ef4..2f13481 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -36,7 +36,7 @@ Only `w:instrText` is known to the library today. A `replace()` targeting text i ### 75. Author-filtered bulk resolution must not touch other authors' revisions [data loss; after #73 step 9] -`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `` before A's `` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documented this and a test pinned it as expected — not acceptable for a data-loss path (found by CodeRabbit on PR #75; verified 2026-08-30). **Fixed in PR #96**, by scoping the w:id index to the author being resolved rather than the filed suggestion of resolving element identities: `Revision` carries no element handle, so identity would have meant a wider signature change for the same outcome — a same-id element of the *same* author still resolves in one pass either way. The pinning test is flipped. The per-id verbs keep the defect; that is #83. +`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `` before A's `` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documented this and a test pinned it as expected — not acceptable for a data-loss path (found by CodeRabbit on PR #75; verified 2026-08-30). **Fixed in PR #96**, by scoping the w:id index to the author being resolved rather than the filed suggestion of resolving element identities: `Revision` carries no element handle, so identity would have meant a wider signature change for the same outcome — a same-id element of the *same* author still resolves in one pass either way. The pinning test is flipped. The per-id verbs keep the defect; that is #84. ### 79. Inserted text loses formatting after an empty split segment [after #73 step 7] @@ -54,18 +54,22 @@ Every path that decides which `rPr` a newly inserted run copies picks its bounda The #91 diagnostics caught it on their first CI failure (run 33331432527, load average 1.14 — an idle runner): the kernel wrote `kernel.json` in 0.5 s, then our `kernel_info` probe on the control channel raised inside ipykernel's ZMQStream callback — `KeyError: '_control_lock'` (traitlets) — because the control message arrived before the kernel finished initializing; the callback's exception left the control stream broken and the kernel never replied within the remaining 59.5 s. So the residual "flake" is a race our own probe triggers, not load. Fix directions, in preference order: probe on the shell channel or wait for the iopub `status: starting` message before touching control; retry with a *fresh* client (the broken stream may be permanent for that connection); check whether current ipykernel has fixed `_control_lock` initialization order and pin/upgrade. Regression test: a kernel whose control channel receives `kernel_info` immediately after `kernel.json` appears must still become ready. -### 83. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss] +### 84. `accept_revision`/`reject_revision` resolve a duplicated `w:id` by position [data loss] The per-id half of #75, which fixed only `accept_all`/`reject_all`. With B's `` before A's ``, `list_revisions(author="A")` returns one row with id 7, and `reject_revision(7)` deletes B's inserted text and leaves A's deletion pending; `accept_revision(7)` makes B's insertion permanent. The caller cannot disambiguate — `Revision` carries no element handle — and `SKILL.md` documents this exact per-id pattern. Fix: either give the per-id verbs an author/identity selector, or refuse an ambiguous id with a teaching error. The duplicate set is already computed — `duplicate_ids` in `_reconstruct_groups` (`registry.py`) — and then discarded; retain or recompute it rather than testing `_revision_groups[id] is None`, which `_adopt_split_tail` also writes for an unambiguous split tail whose origin was ungroupable. SKILL.md carries an interim caveat. Found by the multi-review of PR #96. -### 84. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss] +### 85. `_rejoin_paragraph` drops the tail paragraph's other revisions [data loss] Rejecting a paragraph-mark insertion migrates only the mark `w:ins` off the tail paragraph and then drops that paragraph's whole `w:pPr`, so another author's paragraph-mark `w:del`/`w:moveFrom`/`w:moveTo` and their `w:pPrChange` are destroyed — uncounted, unwarned, and with the merged paragraph's break left permanently untracked. Reproduces with no author filter, and is order-dependent within a single author: the same document returns 1 or 2 depending only on whether the mark-ins id sorts above the mark-del id. The `accept_all` docstring names only the `w:pPrChange` case. Fix: migrate the whole downstream mark set onto the surviving paragraph's mark `rPr`, since they all describe the break it now owns. Found by the multi-review of PR #96. -### 85. Historical marks inside change records list as live revisions +### 86. Historical marks inside change records list as live revisions `list_revisions` and `_revision_element_index` walk with `skip_change_records=False` while `_unhandled_elements` uses `True`, so a revision mark recorded *inside* a change record is offered as adjudicable. `w:pPr/w:rPr/w:rPrChange/w:rPr/w:ins` is conforming (CT_ParaRPrOriginal admits EG_ParaRPrTrackChanges) and lists as a live insertion, and resolving it mutates the other author's change record; the nonconforming `w:pPrChange`-holding-`w:rPr` shape is worse — it merges two live paragraphs. Fix: have both walks pass `skip_change_records=True` and leave recorded marks to the honesty floor. Needs its own regression test. Found by the multi-review of PR #96. +### 87. An unhandled mark's `w:id` can resolve a handled revision [data loss] + +`list_unhandled_revisions()` reports a numeric `w:id` when the mark has one, and ids are unique only per producer: with `` alongside ``, `reject_revision(row.id)` returns True, deletes A's inserted text and leaves the `w:rPrChange` standing — the caller adjudicated a revision it never asked about while the one it named survives. `UnhandledRevision`'s docstring claimed this could only fail; it now says what actually happens. Not reproduced on the corpus (0 handled/unhandled id collisions across 77 files), so it is a latent shape, not an observed one. Fix: have the per-id verbs verify the located element is the one the id was read from, or refuse when a handled and an unhandled element share the id. Related to #84, which is the same hazard between two *handled* rows. Found by the multi-review of PR #96. + ### 69. Return-leg reconciliation: which changesets survived? [design] The product loop is agent proposes → human adjudicates in Word → document comes back; accepted revisions become plain text, rejected ones vanish, and there is no way to ask "which of my changesets survived?". A sent-vs-returned compare keyed on changeset `w:date` + content. Design first; large. diff --git a/docs/api.md b/docs/api.md index 8c5e872..c91fdb1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1089,10 +1089,12 @@ marker is dropped without merging or splitting paragraphs. Only `word/document.xml` is inspected; headers, footers and footnotes are the container-parts epic (ROADMAP.md #30). -An `author=` call resolves that author's own elements and no one else's, even +An `author=` call targets that author's own elements and no one else's, even where a producer repeats one `w:id` across authors (LibreOffice does): the id lookup is scoped to the author being resolved, so another author's same-id -revision is never a candidate. +revision is never a candidate. That is about which elements are *targeted*, not +about what survives — rejecting an insertion still removes whatever another +author nested inside it, `reject_all(author=…)` included. **Parameters:** @@ -1123,8 +1125,9 @@ Reject every listed revision. Resolves insertions, deletions, content moves (both halves as a unit — the text is back at its source exactly once) and paragraph-property changes (the recorded previous properties are restored); every other revision type is left -pending and reported exactly as in `accept_all()`. `author=` is author-exact in -the same way, duplicate `w:id`s across authors included. +pending and reported exactly as in `accept_all()`. `author=` targets one +author's elements in the same way, duplicate `w:id`s across authors included — +and carries away nested revisions in the same way too. **Parameters:** diff --git a/docx_editor/track_changes/models.py b/docx_editor/track_changes/models.py index 07ea9cc..1762972 100644 --- a/docx_editor/track_changes/models.py +++ b/docx_editor/track_changes/models.py @@ -925,8 +925,12 @@ class UnhandledRevision: """One revision element this library does not accept or reject. Returned by ``Document.list_unhandled_revisions()``. Deliberately not a - :class:`Revision`: these carry no adjudicable id, so handing them to - ``accept_revision()`` could only fail — see ``list_unhandled_revisions``. + :class:`Revision`: nothing here is adjudicable, so there is no id to hand + ``accept_revision()`` — see ``list_unhandled_revisions``. Where ``id`` is + not None it is reported for identification only: an unhandled mark's + ``w:id`` may coincide with a *handled* revision's, and passing it to + ``accept_revision``/``reject_revision`` then resolves that other element + instead (ROADMAP.md #87). Attributes: tag: the element's tag — one of ``UNHANDLED_REVISION_TAGS``, or a From 4b1f4c8c765f149a99b40a56e701fb2a6e8e0a43 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 01:43:50 +0200 Subject: [PATCH 12/14] docs: say what author= guarantees, and what it does not (ROADMAP.md #75) The accept_all/reject_all docstrings and SKILL.md promised a filtered call "resolves only that author's own elements". Scoping decides which elements are targeted, not what survives: rejecting an insertion still carries away revisions another author nested inside it, reject_all(author=...) included. docs/api.md was corrected already; these are the sources it mirrors. _resolve_all's index bullet claimed the call "resolves exactly the rows it listed". It can resolve fewer -- a listed row nested inside a rejected insertion goes with its host -- so the honest claim is the negative one: it cannot resolve a row it did not list. The test named for the false generalization is renamed and says why equality holds in its own fixture. _revision_element_index's "a duplicated id resolves the same element whichever path finds it" predates the author parameter and is now false by design: the scan hands back the first element carrying the id, a scoped index the first one that author wrote. It is scoped to the unscoped case. accept_revision/reject_revision gain the duplicate-w:id caveat that SKILL.md and docs/api.md already carry. --- docx_editor/track_changes/listing.py | 16 ++++++++++++--- docx_editor/track_changes/resolution.py | 27 +++++++++++++++++++------ skills/docx/SKILL.md | 8 +++++--- tests/test_track_changes.py | 22 +++++++++++--------- 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/docx_editor/track_changes/listing.py b/docx_editor/track_changes/listing.py index 8827395..bc7b366 100644 --- a/docx_editor/track_changes/listing.py +++ b/docx_editor/track_changes/listing.py @@ -164,7 +164,9 @@ def get_markup_text(self) -> str: ``[ins#{id}:{author}]...[/ins]`` / ``[del#{id}:{author}]...[/del]``, where ``{id}`` is the id ``accept_revision``/``reject_revision`` take (so a raw ``w:id="007"`` renders ``#7``) and ``#?`` marks one they - cannot reach at all — those are ``list_unhandled_revisions()`` rows — + cannot reach at all (outside a change record those are + ``list_unhandled_revisions()`` rows; a mark recorded *inside* one + renders here but is not listed there — ROADMAP.md #86) — nesting included (e.g. ``[ins#1:A]kept [del#9:B]gone[/del][/ins]``). The two halves of a content move render the same way as ``[moveFrom#{id}:{author}]...[/moveFrom]`` / @@ -326,8 +328,11 @@ def _revision_element_index(self, author: str | None = None) -> dict[str, list[E id-based can reach it, and ``_unhandled_elements`` is what reports it. Each id's list is in document order — the same order the fresh scan - in ``_find_revision_element`` uses, so a duplicated id resolves the - same element whichever path finds it. + in ``_find_revision_element`` uses, so an *unscoped* index resolves a + duplicated id to the same element the scan would. A scoped one + deliberately answers differently, which is the whole point of + ``author``: the scan would hand back the first element carrying the + id, the scoped index the first one this author wrote. """ element_index: dict[str, list[Element]] = {} for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): @@ -375,6 +380,11 @@ def _find_revision_element( (``_revision_element_index(author)``) holds only that author's, so a filtered call cannot land on another author's same-id revision. + ``revision_id`` is typed wider than the public verbs' ``int`` because + this is where an id is *checked*, not promised: ``UnhandledRevision.id`` + is ``int | None``, so None reaches here in practice and has to resolve + nothing rather than match the first unreachable mark. + Both paths compare ``str`` of the *adjudicable* id, which is what keeps them answering alike. Comparing the parsed int on one side and the index's string key on the other would diverge on everything int-equal diff --git a/docx_editor/track_changes/resolution.py b/docx_editor/track_changes/resolution.py index dd4d902..2765c23 100644 --- a/docx_editor/track_changes/resolution.py +++ b/docx_editor/track_changes/resolution.py @@ -43,6 +43,12 @@ def accept_revision(self, revision_id: int, element_index: dict[str, list[Elemen content is all gone are swept (``_sweep_move_range_marks``) — move content can leave inside any resolved host, not only a move half. + A duplicated w:id is ambiguous here: Word does not guarantee w:id is + unique, and this call has only the id, so it resolves whichever element + carries it first in the document — possibly another author's + (ROADMAP.md #84). Two rows sharing an id in ``list_revisions()`` is the + signal; ``accept_all(author=...)`` is author-exact where this is not. + Args: revision_id: The w:id of the revision to accept element_index: Optional pre-built w:id -> element map (see @@ -99,6 +105,10 @@ def reject_revision(self, revision_id: int, element_index: dict[str, list[Elemen A paragraph-mark move marker is dropped, as on accept. + A duplicated w:id is ambiguous here in the same way ``accept_revision`` + describes: with only the id to go on, this resolves whichever element + carries it first in the document (ROADMAP.md #84). + Args: revision_id: The w:id of the revision to reject element_index: Optional pre-built w:id -> element map (see @@ -285,8 +295,9 @@ def _resolve_all(self, author: str | None, resolve: Callable[[int, dict[str, lis pass. A filtered call instead indexes only ``author``'s elements (``_revision_element_index(author)``), so another author's element sharing that id is never a candidate: the listing and the index - agree on ownership, and the call resolves exactly the rows it - listed. + agree on ownership, and the call cannot resolve a row it did not + list. It may resolve fewer — a listed row nested inside a rejected + insertion goes with its host — but never someone else's. The index is built once for the whole call, not per pass: resolution only ever detaches elements, so a single build stays a valid superset @@ -357,9 +368,11 @@ def accept_all(self, author: str | None = None) -> ResolveResult: ``_resolve_all``, which re-lists on each pass: that fully resolves *nested* revisions in Word-authored files (e.g. a w:del inside a w:ins) and terminates even when an author filter leaves other authors' - revisions in the document. An ``author=`` call resolves only that + revisions in the document. An ``author=`` call *targets* only that author's own elements even when a producer repeats a w:id across - authors: the id lookup is scoped to the author being resolved. + authors: the id lookup is scoped to the author being resolved. That is + about which elements are targeted, not about what survives — rejecting + an insertion still removes whatever another author nested inside it. A move is resolved as a unit: its ``w:moveFrom`` content is removed and its ``w:moveTo`` content unwrapped in the same call, and the range @@ -407,9 +420,11 @@ def reject_all(self, author: str | None = None) -> ResolveResult: ``_resolve_all``, which re-lists on each pass: that fully resolves *nested* revisions in Word-authored files (e.g. a w:del inside a w:ins) and terminates even when an author filter leaves other authors' - revisions in the document. An ``author=`` call resolves only that + revisions in the document. An ``author=`` call *targets* only that author's own elements even when a producer repeats a w:id across - authors: the id lookup is scoped to the author being resolved. + authors: the id lookup is scoped to the author being resolved. That is + about which elements are targeted, not about what survives — rejecting + an insertion still removes whatever another author nested inside it. A move is undone as a unit: its ``w:moveTo`` content is removed and its ``w:moveFrom`` content restored in place, range marks swept — the diff --git a/skills/docx/SKILL.md b/skills/docx/SKILL.md index 4cf5874..e75d782 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -675,9 +675,11 @@ nested deletion's `nested_under` points back at its host. - `accept_all()` / `reject_all()` resolve nesting fully (they re-scan until no listed revision remains), and `author=` filters process each author's changes independently — including when a producer repeats a `w:id` across - authors, where a filtered call still resolves only that author's own - elements. Revision types outside `list_revisions()` are not resolved at - all — see the `result.unhandled` recipe above. + authors, where a filtered call still *targets* only that author's own + elements. Targeting, not survival: as above, rejecting an insertion still + carries away what another author nested inside it. Revision types outside + `list_revisions()` are not resolved at all — see the `result.unhandled` + recipe above. - That duplicate-`w:id` guarantee is the bulk calls' alone. A producer may reuse one `w:id` across authors, and `accept_revision(id)`/ `reject_revision(id)` take only the id: handed a duplicated one they resolve diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index d3839ea..c8c4cb4 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1282,12 +1282,14 @@ def test_accept_all_unfiltered_still_resolves_both_same_id_revisions(self): assert dom.getElementsByTagName("w:delText") == [] assert [t.firstChild.data for t in dom.getElementsByTagName("w:t")] == ["BEE"] - def test_accept_all_author_filter_resolves_the_rows_it_listed(self): - """Test that a filtered call's count equals the number of that author's listed rows. + def test_accept_all_author_filter_resolves_every_row_it_listed(self): + """Test that a filtered call resolves every one of that author's listed rows. Index and listing must agree on ownership: if the index used a different author expression, the filtered pass would list rows it could not resolve and exit on the no-progress guard — a silent no-op. + Equality holds here because none of these rows is nested; a listed row + inside a rejected insertion goes with its host and is not counted. """ manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS) @@ -1302,16 +1304,18 @@ def test_author_filter_holds_when_ids_collide_only_after_normalizing(self): one index key, so normalizing the key widened the duplicate-id class this fix guards. The author scoping has to cover the widened class too. """ + # B's non-canonical id comes *second*: an unscoped index would hand + # back A's element first, so the assertions below need the scoping as + # well as the normalization. manager = _make_revision_manager( """ - BEE - AYE""" + AYE + BEE""" ) dom = manager.editor.dom - # Resolve B, whose id is the non-canonical one: it is reachable only - # because the index normalizes, and author-exact only because the - # index is scoped -- so this needs both halves to hold. + # Resolving B needs both halves: reachable only because the index + # normalizes, author-exact only because the index is scoped. assert manager.reject_all(author="B") == 1 # A's deletion is untouched: still pending, its text still deleted. @@ -1401,8 +1405,8 @@ def test_none_id_resolves_nothing(self, method): ``list_unhandled_revisions()`` reports ``id=None`` for a mark with no numeric ``w:id``, so None is a value callers really do hold. The fresh - scan compares parsed ids, and ``_adjudicable_id`` is None for exactly - those marks — unguarded, None would match the first of them and + scan skips elements whose ``_adjudicable_id`` is None; without that + skip ``str(None) == str(None)`` would match the first such mark and resolve what the honesty floor just called unresolvable. """ manager = _make_revision_manager('KEEP') From 45629698c8393cd5af479d0b1263fc1554334f4b Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 01:43:50 +0200 Subject: [PATCH 13/14] test: pin group liveness for a non-canonical member id (ROADMAP.md #75) groups_are_dead answers through the same w:id index resolution uses, so a group whose member carries w:id="007" was reported dead while it was live -- and a dead group has nothing left to explain, which is what deletes its note= comment. Nothing covered it; reverting the index key to the raw attribute now fails this test. _adopt_split_tail reads its origin id through _adjudicable_id like the rest of the registry. ROADMAP #75 keeps the one-line record of the fix and leaves the design rationale to the PR. --- ROADMAP.md | 2 +- docx_editor/track_changes/registry.py | 8 ++++---- tests/test_revision_groups.py | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 2f13481..686c440 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -36,7 +36,7 @@ Only `w:instrText` is known to the library today. A `replace()` targeting text i ### 75. Author-filtered bulk resolution must not touch other authors' revisions [data loss; after #73 step 9] -`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `` before A's `` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documented this and a test pinned it as expected — not acceptable for a data-loss path (found by CodeRabbit on PR #75; verified 2026-08-30). **Fixed in PR #96**, by scoping the w:id index to the author being resolved rather than the filed suggestion of resolving element identities: `Revision` carries no element handle, so identity would have meant a wider signature change for the same outcome — a same-id element of the *same* author still resolves in one pass either way. The pinning test is flipped. The per-id verbs keep the defect; that is #84. +`accept_all(author=A)` / `reject_all(author=A)` list revisions by author but resolve by `w:id`, and the id lookup has no author check. With B's `` before A's `` (duplicate ids across authors occur in real files — the corpus has them from LibreOffice), `reject_all(author="A")` deletes B's inserted text and `accept_all(author="A")` makes it permanent, both reporting 2. The `accept_all` docstring documented this and a test pinned it as expected — not acceptable for a data-loss path (found by CodeRabbit on PR #75; verified 2026-08-30). **Fixed in PR #96** (the w:id index is scoped to the author being resolved; the pinning test is flipped). The per-id verbs keep the defect; that is #84. ### 79. Inserted text loses formatting after an empty split segment [after #73 step 7] diff --git a/docx_editor/track_changes/registry.py b/docx_editor/track_changes/registry.py index f927e89..465aae9 100644 --- a/docx_editor/track_changes/registry.py +++ b/docx_editor/track_changes/registry.py @@ -278,10 +278,10 @@ def _adopt_split_tail(self, original_ins, new_nodes) -> None: operation — otherwise rejecting that operation's group would rip a leftover piece out of a pre-existing insertion. """ - try: - origin_group = self._revision_groups.get(int(original_ins.getAttribute("w:id"))) - except ValueError: # pragma: no cover - our own ins always has a numeric id - origin_group = None + # Our own ins always has a numeric id; a non-numeric one is simply + # ungrouped, the same answer .get would give for an unknown id. + origin_id = _adjudicable_id(original_ins) + origin_group = None if origin_id is None else self._revision_groups.get(origin_id) for node in new_nodes: if node.nodeType == node.ELEMENT_NODE and node.tagName == "w:ins": # pragma: no branch tail_id = int(node.getAttribute("w:id")) diff --git a/tests/test_revision_groups.py b/tests/test_revision_groups.py index 22e49de..c6fb7e7 100644 --- a/tests/test_revision_groups.py +++ b/tests/test_revision_groups.py @@ -942,6 +942,25 @@ def test_unknown_live_and_resolved_groups(self, temp_xml): assert manager.groups_are_dead([1, 2]) == {1} assert set(manager.group_spans([1, 2])) == {2} + def test_group_with_a_non_canonical_member_id_reads_as_live(self, temp_xml): + """A live group whose member's raw w:id is not str(int) is not dead. + + Liveness is answered through the same w:id index resolution uses. Keyed + on the raw attribute, a ``w:id="007"`` member is listed as 7 and found + under neither key, so a live group read as dead — and a dead group has + nothing left to explain, which is what deletes its ``note=`` comment. + """ + body = '' + body += "one" + manager = _make_manager(temp_xml(body)) + (rev,) = manager.list_revisions() + assert rev.id == 7 and rev.group_id is not None + + assert manager.groups_are_dead([rev.group_id]) == set() + + assert manager.accept_group(rev.group_id) == 1 + assert manager.groups_are_dead([rev.group_id]) == {rev.group_id} + def test_paragraph_mark_only_group_is_live_but_has_no_span(self, temp_xml): # A pure tracked split: the group's only member is a paragraph-mark # insertion, which cannot hold a comment marker. The group is alive From 534bb09d3a146a207181be7392a4ab6b6245a57c Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Mon, 31 Aug 2026 02:24:37 +0200 Subject: [PATCH 14/14] docs: keep the targeting/survival distinction in _resolve_all (ROADMAP.md #75) "never someone else's" still read as a survival guarantee where its siblings had already been qualified: another author's revision nested inside a rejected insertion does go with it, uncounted. test_accept_all_author_filter_resolves_every_row_it_listed credited itself with covering the author expression, which its fixture cannot -- both rows carry an explicit w:author, so the "Unknown" fallback is pinned by test_unattributed_revision_resolves_under_the_unknown_author instead. It now says which half it covers and points at the other. --- docx_editor/track_changes/resolution.py | 4 +++- tests/test_track_changes.py | 13 ++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/docx_editor/track_changes/resolution.py b/docx_editor/track_changes/resolution.py index 2765c23..f03835c 100644 --- a/docx_editor/track_changes/resolution.py +++ b/docx_editor/track_changes/resolution.py @@ -297,7 +297,9 @@ def _resolve_all(self, author: str | None, resolve: Callable[[int, dict[str, lis sharing that id is never a candidate: the listing and the index agree on ownership, and the call cannot resolve a row it did not list. It may resolve fewer — a listed row nested inside a rejected - insertion goes with its host — but never someone else's. + insertion goes with its host — but never counts someone else's. + Targeting, not survival: another author's revision nested inside a + rejected insertion still goes with it, uncounted. The index is built once for the whole call, not per pass: resolution only ever detaches elements, so a single build stays a valid superset diff --git a/tests/test_track_changes.py b/tests/test_track_changes.py index c8c4cb4..07ae807 100644 --- a/tests/test_track_changes.py +++ b/tests/test_track_changes.py @@ -1285,11 +1285,14 @@ def test_accept_all_unfiltered_still_resolves_both_same_id_revisions(self): def test_accept_all_author_filter_resolves_every_row_it_listed(self): """Test that a filtered call resolves every one of that author's listed rows. - Index and listing must agree on ownership: if the index used a - different author expression, the filtered pass would list rows it - could not resolve and exit on the no-progress guard — a silent no-op. - Equality holds here because none of these rows is nested; a listed row - inside a rejected insertion goes with its host and is not counted. + A filtered pass that could not reach a row it listed would exit on the + no-progress guard and report a count below the listing — a silent + no-op. Equality holds here because none of these rows is nested; a + listed row inside a rejected insertion goes with its host and is not + counted. The other way index and listing can disagree about ownership + — the ``"Unknown"`` fallback for a missing ``w:author`` — is pinned by + ``test_unattributed_revision_resolves_under_the_unknown_author``; both + of these rows carry an explicit author. """ manager = _make_revision_manager(self.DUPLICATE_ID_ACROSS_AUTHORS)