fix: don't resolve another author's same-id revision (ROADMAP.md #75) (#96)

* 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.

* 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.

* 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.

* 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.

* 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.

* 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

* 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.

* 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.

* 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.

* 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.

* 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.

* 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.

* 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.

* 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.

Author: pablospe

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 `<w:ins w:id="7">` before A's `<w:del w:id="7">` (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 `<w:ins w:id="7">` before A's `<w:del w:id="7">` (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 `<w:ins w:id="7">` before A's `<w:del w:id="7">`, `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 `<w:rPrChange w:id="7" w:author="B">` alongside `<w:ins w:id="7" w:author="A">`, `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.

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:**
 

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
 

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.