diff --git a/ROADMAP.md b/ROADMAP.md index 1ec301f..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 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** (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] @@ -54,6 +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. +### 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. + +### 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. + +### 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 932ee37..c91fdb1 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,13 @@ 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 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. 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:** - `author` (str, optional): If provided, only accept revisions by this author @@ -1109,7 +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()`. +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/base.py b/docx_editor/track_changes/base.py index 582cc18..95e3a55 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: @@ -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/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 5e4a7f1..bc7b366 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, @@ -70,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 @@ -157,6 +162,11 @@ 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 (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]`` / @@ -182,8 +192,12 @@ 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 "?" - rev_author = child.getAttribute("w:author") or "Unknown" + # 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"): parts.append(get_text_node_data(child)) @@ -219,7 +233,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 @@ -272,7 +286,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,18 +306,42 @@ 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. 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 - 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): - element_index.setdefault(elem.getAttribute("w:id"), []).append(elem) + rev_id = _adjudicable_id(elem) + if rev_id is None: + continue + 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: @@ -325,7 +363,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``. @@ -337,15 +375,34 @@ 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. + + ``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 + 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: - wanted = str(revision_id) for elem in iter_revision_elements(self.editor.dom, HANDLED_REVISION_TAGS): - if elem.getAttribute("w:id") == wanted: + 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 @@ -379,7 +436,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. @@ -421,7 +478,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/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 diff --git a/docx_editor/track_changes/registry.py b/docx_editor/track_changes/registry.py index 854824d..465aae9 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 @@ -282,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")) @@ -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: diff --git a/docx_editor/track_changes/resolution.py b/docx_editor/track_changes/resolution.py index 802ed8b..f03835c 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 @@ -154,6 +164,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 +290,16 @@ 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 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 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 @@ -284,7 +309,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 +370,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. 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 *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. 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 @@ -395,9 +422,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. 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 *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. 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 cd499a4..e75d782 100644 --- a/skills/docx/SKILL.md +++ b/skills/docx/SKILL.md @@ -674,8 +674,20 @@ 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 *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 + whichever element comes first in the document, which may be the other + 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 diff --git a/tests/test_revision_groups.py b/tests/test_revision_groups.py index e1f2697..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 @@ -1687,6 +1706,73 @@ 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"{_ins_xml(7, 'BEE', author=AUTHOR_B, date=DATE_B)}{_del_xml(7, '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_group_or_changeset_id_still_raises(self, temp_xml, method): + manager = _make_manager(temp_xml(self.DUPLICATE_ID_BODY)) + + # 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)(allocated + 1000) + + 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_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 73945a9..07ae807 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,254 @@ 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_every_row_it_listed(self): + """Test that a filtered call resolves every one of that author's listed rows. + + 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) + + listed = len(manager.list_revisions(author="B")) + + 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. + """ + # 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( + """ + AYE + BEE""" + ) + dom = manager.editor.dom + + # 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. + remaining = manager.list_revisions(author="A") + assert len(remaining) == 1 + 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): + """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 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. + + 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"] + + @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 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') + (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"] + + @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."""